Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Serving "Virtual" Routes from Plugins #1737

Closed
illBeRoy opened this issue Jan 29, 2022 · 18 comments
Closed

Allow Serving "Virtual" Routes from Plugins #1737

illBeRoy opened this issue Jan 29, 2022 · 18 comments

Comments

@illBeRoy
Copy link
Contributor

Feature request

Plugins are a great way to introduce dynamic content into existing pages. But, what if we took this a step further, and allowed plugins to actually generate the routes themselves on the fly?

The feature I want to contribute introduces an API for plugins to actually provide dynamically generated pages for routes, even if actual markdown files do not exist to begin with.

What problem does this feature solve?

Say that you have a list of APIs, which can change at any time. Each one should have its own page under /apis/<METHOD_NAME>.md.

Today, you will have to create real markdown files and deploy them along with your site, as every route in a Docsify website requires that a file exist on the server. This means that you will have to create all the /apis/*.md files manually and deploy them, and do this every time that even a character changes in any of the API descriptions.

With my proposed solution, you will be able to create a plugin that intercepts all fetches from /apis/*, and provide dynamically generated markdown as replacement from fetching real markdown files from the server.

What does the proposed API look like?

Add a new hook to the Plugin API, called: fetchRoute(cb), which will essentially allow plugins to intercept fetch requests before they are made by the underlying engine. If any plugin has a fetchRoute hook, and the callback provided any content for the route, use that content instead of fetching.

The fetchRoute Hook Behavior

This hook receives a callback, possibly async, that implements the following signature: (route: Route, next: fn) => void | Promise<void>. The route object is the same one that can be found on vm.route.

The next function can either receive a markdown string, or nothing at all. If a non-empty string was provided, then this is the markdown that will be passed into the system, and no fetch will be done by the Fetch class itself.

If the next function was called with a falsy value, it will just call the next plugin that has fetchRoute. Finally, if all plugins were exhausted and none returned any markdown, the internal fetch function will be run like today.

Flow Diagram

image

image

image

How should this be implemented in your opinion?

From my brief introduction to the code, I would suggest that we edit the _fetch method (link to code), so instead of directly using get, it will run the fetchRoute hooks first and only fall back to get if none return markdown.

Are you willing to work on this yourself?

Yes :) Would love to.

@illBeRoy
Copy link
Contributor Author

Hey guys,

is there anyone I can tag in order to get this proposal reviewed?
Maybe @jhildenbiddle ? (based on other issues I've seen?)

Thanks in advance for the feedback :)

@sy-records
Copy link
Member

cc @docsifyjs/reviewers

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Feb 18, 2022

Thanks for taking the time to document the request, @illBeRoy. This looks interesting.

I haven't had time to think this through, but here are a few thoughts off the top of my head and in no particular order:

  • The proposed fetchRoute hook name feels inconsistent with the existing hook names. To be honest, I don't love the existing hook names and they too lack consistency, so my feedback here is admittedly about remaining "consistent with our inconsistency" versus finding the best name possible. A name like beforeRequest feels like it aligns better with the current hook names. It also has the benefit of not implying a request method (fetch vs. XMLHttpRequest), which could otherwise cause confusion since the proposed name contains fetch but we're using XMLHttpRequest under the hood.
  • The proposed hook is also the logical place to programmatically modify request options. For example, plugins could modify the request URL, headers, if/how credentials are sent, etc. To facilitate this, the next() function could receive a modified route object in additional to a markdown string. The shape of the route object should be documented, agnostic from existing web APIs, and does not need to support every fetch() or XMLHttpRequest option available (IMHO).
  • Are there any scenarios where _fetch() is used to request content that we would not want to send through the plugin chain? My first concern was with embedded files, but it appears this is not a concern from the limited testing I did. I have not spent a lot of time with part of the codebase so this may be a non-issue, but I thought it was worth mentioning.

While outside the scope of this proposal, it's easy to imagine how creating dynamic site content would also be useful for other docsify content such as the top navigation, sidebar, and embeds. Multiple feature requests have been made here and in other repos to allow markdown features like helpers, plugins, and Vue rendering to just work everywhere rather than only in the main content area. Obviously, docsify isn't setup to do that today and doing so would require significant changes, but it's worth considering as we explore how to integrate the proposed feature. For example, how could a plugin author use beforeRequest to dynamically generate sidebar content? How about generating content from an embedded file so pages can render both static and dynamic content instead of one or the other?

Exciting stuff. Looking forward to hearing everyone's thoughts. :)

@trusktr
Copy link
Member

trusktr commented Feb 18, 2022

I think fetchRoute is a better name than beforeRequest. They imply entirely different things. beforeRequest implies that it runs before a request, which is not what we want, and also begs the question "what about afterRequest"?

I propose we not introduce new names that follow naming of existing hooks, and make new hooks have the most meaningful name they can have.

Also I don't think fetchRoute is confusing. The plugin author can fetch any way they want, whether with fetch or not. It's just a verb.

Naming ideas:

  • fetchRoute
  • getRoute
  • requestRoute
  • onRoute
  • provideRouteContent
  • provideRoute (my favorite, so far, it doesn't imply fetching)

Note that someone doesn't even have to fetch anything, they could just provide a hard coded markdown string for example.

My first concern was with embedded files,

I propose let's not worry about this yet. A first version can allow a plugin to provide markdown for the top level route only, not all other requests. I believe this is what the OP is about too.

After that, embedded files would work the same.

Later on, as a separate feature, onRequest or onResponse hooks could be used to intercept and, if desired by a plugin author, replace the responses or requests with alternates.

@trusktr
Copy link
Member

trusktr commented Feb 18, 2022

Or maybe we just need onResponse and onRequest? That would be more powerful, and we can include metadata as to whether a request or response is for a route or not. Or if it is for an embedded file, etc

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Feb 19, 2022

@trusktr - I think fetchRoute is a better name than beforeRequest. They imply entirely different things. beforeRequest implies that it runs before a request, which is not what we want...

Running before a request is the proposed behavior. From the OP:

@illBeRoy - Add a new hook to the Plugin API, called: fetchRoute(cb), which will essentially allow plugins to intercept fetch requests before they are made by the underlying engine.

Instead of focusing on serving virtual routes, I think we're better served by focusing on the "intercept fetch requests before they are made" part of the proposal.

There are many reasons why a site owner or plugin author may want to intercept a request. The OP's use case -- requesting data from an API and generating content from it -- is one of many possible use cases. Other use cases include displaying a UI element, prompting for user input, interacting with a Vue component, changing the state of a remote application, or modifying request headers. This is why I believe a name based on the event/stage (like beforeRequest) is better than a name based on an assumed use case like fetchRoute, provideRouteContent, setRequestHeaders, etc. The words "before" and "request" are the same words used by the OP to describe the event/stage.

@jhildenbiddle - My first concern was with embedded files
@trusktr - I propose let's not worry about this yet.

To clarify, the OP mentioned modifying the existing _fetch() method. My comment was about making sure we understand how this method is used so we don't unintentionally feed the wrong type of fetched data (like embeds) into the plugin chain. I wasn't suggesting that the initial implementation needs to handle embedded files.

My intention in mentioning features "outside the scope of this proposal" was to encourage @illBeRoy to think beyond the original use case. My gut tells me that if someone wants to dynamically generate pages from an API, it won't be long before they want to do the same for navigation (sidebar) or portions of a page (embedded files). Considering this while building v1 can make the road to v2 shorter and less bumpy for everyone. If the decision is "we don't care about now", that's okay, too.

@trusktr - beforeRequest [...] also begs the question "what about afterRequest"?

Yep!

I think afterResponse is more appropriate match for beforeRequest. These hook names are used by many other packages. Got by sindresorhus is one example (see here and here).

@trusktr - Or maybe we just need onResponse and onRequest? That would be more powerful...

Yep! Both a request- and a response-related hook would be useful. Let's do both.

An onRequest hook (outlined above as beforeRequest) accommodates the OP's use case as well as each of the other use cases I mentioned.

An onResponse hook could be used to modify content just like beforeEach is today, but it could also be used to inspect response-related information like headers and status codes not available with beforeEach. We could also provide helper methods that allow for things like initiating a new request at the beginning of docsify's plugin chain to handle certain kinds of errors. We can flush out the details separately, but knowing that a response-related hook is planned will help us better understand and define the request-related hook.

As far as naming goes, the on[Event] naming convention works well for request/response. I think I prefer it over beforeRequest and afterResponse, but only if we include both request/response hooks. Another option is to just use hook.request and hook.response which align with our existing init, mounted, and ready hooks.

Also worth mentioning: In the future we can determine if we need both onResponse and beforeEach, or if a single hook is sufficient since onResponse does everything beforeEach does and more. Short-term, we can simply add onResponse as a non-breaking change and see how it goes. That's all TBD of course, but I wanted to cover the overlap between onResponse and beforeEach since beforeEach kinda/sorta serves as our "response" hook today.

@trusktr - A first version can allow a plugin to provide markdown for the top level route only, not all other requests. I believe this is what the OP is about too.

If by "top level route only" you mean the route associated with the main content area then I agree. Handling the top nav, sidebar, and embeds is a separate effort that I assume would be rolled into a larger plugin system project.

@illBeRoy
Copy link
Contributor Author

Hey guys!

Following the discussion, the way I understand it we have two main options for implementation, each (imo) has a slightly different meaning:

The fetchRoute Option

This was my original proposal: let plugin writers provide virtual routes. It doesn't have a lot to do with request and response, and more like a replacement for the network request altogether. The product here is not to intercept requests; intercepting them is just an implementation details. Instead, the API here is to let you provide custom markdown based on the route, instead of using network requests altogether.

This is a higher level API to allow users to provide custom content for routes.

  • The pros: simple, straightforward API that implements a specific use case (custom routes)
  • The cons: doesn't cover everything. That is, as @jhildenbiddle rightfully noted, any use case that isn't specifically what I described (e.g. custom sidebar \ embedded files \ etc) cannot be covered by this API.

The onRequest and onResponse Option

This option is a different product than my original proposal. Instead of telling users "you can provide custom pages", we're telling them: "you can intercept requests and responses and alter them". Any request and response - not just routes.

This allows for my original use case, but solves it from a different perspective: this time, you let plugins intercept requests and alter them, similar to what Puppeteer's setRequestInterception API does.

This also opens up followup questions:

  1. Should onRequest allow for passing through? (that is, intercept, make some modifications to the request like change target URL, and then let the request carry on)
  2. In such API, what would "canceling" the request look like?
  3. How can we differ between "no interception" to "not found"?
  4. Having both onResponse and beforeEach might be confusing. But this is actually ok IMO, since people can use beforeEach by default (as a high-level API), and onResponse only if needed.

In addition, if we decide to go down this path, we should probably define a full request interface here, similar to what they do in the aforementioned example from Puppeteer: a request object that has .redirect(), .abort(), and .continue() methods.

To summarize:

  • The pros: more powerful, covers more use cases
  • The cons: larger refactor, more complex API

Bottom Line

Option one lets you integrate with "content providers". Option two lets you declare interceptors. Option one provides a higher level API that would be easier to use, but is more limited than option two in what you can achieve.

The way I see it, both options aren't contradicting each other, and can live side by side. Much like onResponse and beforeEach, fetchRoute can be a higher level API for your everyday content providing needs, and onRequest and onResponse can be used for more advanced use cases.

If we go down that path, we can strategically decide how and when to implement each option. We can start by delivering fetchRoute (or call it onRoute) right now, and implement it using the path of least resistance. Down the road, we can independently implement onRequest and onResponse once the need truly arises from the community, and maybe even change fetchRoute to use them internally.

All that said and done, the final say here is yours and I would be more than happy to contribute either option according to your decision :) Looking forward for your feedback!

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Mar 3, 2022

Hey @illBeRoy.

Thanks for the thoughtful breakdown and your willingness to work through the details.

I agree we're talking about two different approaches, both of which address the route-focused use case you've outlined. My concern is how this all plays out if/when request/response hooks are implemented. If we agree that these hooks are useful, I think it makes sense to assume they will be so we can work through potential issues and make better short- and long-term decisions. I also think the pros and cons can easily be made to favor either approach, as can the the rational for implementing one hook before the other.

The good news is that I have a proposal that I believe works well for everyone and addresses the issues discussed so far.

Let's dive in...

1. Add routes configuration option

This is the "higher-level" API for providing dynamic route-based markdown content. Providing this functionality as a configuration option is consistent with docsify's existing route/request-related options like alias, basePath, and requestHeaders.

// Object
window.$docsify = {
  routes: {
    // Basic match w/ return string
    '/foo/': '# Custom Markdown',

    // RegEx match w/ return string and match string
    '/foo/(.*)': '# Route match: $1',

    // RegEx match w/ synchronous function
    '/bar/(.*)': function(route, matched) {
      console.log(`Route match: ${matched[0]}`);
      return '# Custom Markdown';
    },

    // RegEx match w/ asynchronous function
    '/baz/(.*)': function(route, matched, next) {
      console.log(`Route match: ${matched[0]}`);
      next('# Custom Markdown');
    }
  }
}

Benefits over hook-based routes

  • Custom routes without requiring a custom plugin
  • Route configurations available for inspection and modification
  • URL matching handled by docsify

Implementation

  • route argument is a string that reflects changes made by docsify configuration
  • matches argument is an array of RegEx matches (if applicable)
  • Synchronous functions return directly
  • Asynchronous functions return using next()
  • When a route function returns a string of any length, the value is passed to the hook.response plugin chain as responseText without an accompanying xhr object (see below). This accomplishes the following:
    1. Allows custom routes to render an empty content area
    2. Allows handling of response text using hook.response or hook.beforeEach
    3. Allows distinguishing between responseText from a custom route or a request
  • When a route function returns an explicit false, the route change is aborted and no change is made to the window location.
  • When a route function returns no value or any other falsey value, the next available route will be processed. If the current route is the last route defined, the system will make a standard request and proceed as usual.

2. Add request hook

Discussed previously as onRequest. This hook can be used for variety of use cases such as dynamically setting request headers, displaying a UI element, or providing dynamic route-based markdown instead of using the route configuration option.

// Synchronous
hook.request(function(url, xhr) {
  // ...
});

// Asynchronous
hook.request(function(url, xhr, next) {
  // ...
});

Benefits over routes configuration option

  • Route configuration can be protected from inspection and modification

Implementation

The behavior is similar to custom route functions with a few key differences:

  • When the hook returns no value or a falsey value other than false or an empty string, the next hook.request function will be invoked. If the current hook is the last hook in the chain, the system will make a standard request and proceed as usual.

3. Add response hook

Discussed previously as onResponse. This hook is essentially the same as the beforeEach but with access to additional response data via the xhr object (headers, status code, status text, etc).

// Synchronous
hook.response(function(responseText, xhr) {
  // ...
});

// Asynchronous
hook.response(function(responseText, xhr, next) {
  // ...
});

Benefits over beforeEach hook

  • XMLHttpRequest object available for response inspection and modification

Implementation

The behavior is similar to the request hook with a few key differences:

  • When a hook returns a string of any length, the value is passed to the hook.beforeEach plugin chain as content.
  • When the hook returns no value or a falsey value other than false or an empty string, the next hook.response function will be invoked. If the current hook is the last hook in the chain, the value will be passed to the first b beforeEach hook and the system will proceed as usual.

Notes

  1. We can remove the "on" prefix from the request/response hook names. It doesn't make the hook names more descriptive and dropping it provides consistency with existing hooks like hook.init, hook.mounted, and hook.ready (all of which could be prefixed with on but currently are not).

  2. In the future we can revisit the overlap between the response and beforeEach hooks as well as hook names.

  3. The OP originally mentioned passing the vm.route object as as the route object to custom route functions. vm.route data does not reflect route changes made by docsify configuration options like alias, basePath, and relativePath, while the url value passed to request hook will reflect these changes.

    window.$docsify = {
      basePath: '/foo/'
    }

    vm.route object

    {
      path: "/",
      file: "README.md",
      query: ""
    }

    url value passed to request hook:

    "/foo/README.md"
    

    Passing different values to the custom route functions and request hooks will likely cause some confusion. This is why I specified that both the route and url value passed to these functions should reflects changes made by docsify configuration.

Proposal

  1. Implement the routes configuration option above as a stand-alone PR. Users and plugin-authors will then be able to add, remove, and modify custom routes by modifying the routes configuration object.
  2. After merging the routes configuration option, implement both the request and response hooks in a separate PR.
  3. Publish new hooks as a minor update (non-breaking changes)
  4. 🥳

@jhildenbiddle
Copy link
Member

@illBeRoy --

Checking in. 😄

@illBeRoy
Copy link
Contributor Author

Hey! Sorry for the delay, seems like I completely missed the previous update!

This looks like a great idea. Gonna start working on the first PR asap.

@jhildenbiddle
Copy link
Member

@illBeRoy --

Sounds great. Looking forward to it. Happy to lend a hand if needed.

@illBeRoy
Copy link
Contributor Author

illBeRoy commented May 14, 2022

@jhildenbiddle

Hey again!

I know it's been a while (had some stuff going on and almost no free time on my hands), but I finally got to work on the PR and I'm almost ready to submit!

Before I do, though, I've a few questions about the spec, and would love your feedback:

Matches in string routes

Regarding the following:

{
  // RegEx match w/ return string and match string
  '/foo/(.*)': '# Route match: $1',
}

While it seems like an extremely convenient way to format routes without having to write too much code, I'm a little worried that people might unknowingly trigger this mechanism.

Now, I thought about possible solutions (e.g. more distinct delimiter; a standard templating language like ejs or vm) but IMO that inherent problem is that people who do not expect it might still fall for it.

Bottom Line

I already implemented this feature, but maybe we should drop it altogether? If someone wants to format the match into their pages, let them use a function and not a string.

The "next" function

I was thinking that maybe we can rethink how we handle async functions? That is, if a function is sync, we just use its return value as the contents of the page.

How about we do the same with async functions, instead of making you invoke a next function? API seems a little bit more straightforward imo.


Thanks for your time! LMK what you think and I'll adjust the PR accordingly.

@jhildenbiddle
Copy link
Member

jhildenbiddle commented May 17, 2022

@illBeRoy --

Very excited to hear about your progress! Thanks for devoting the time and energy on this. Very much appreciated. :)

While it seems like an extremely convenient way to format routes without having to write too much code, I'm a little worried that people might unknowingly trigger this mechanism.

If I understand the concern correctly, the issue is that a string-based route value my contain $1 which may unintentionally be replaced with the matched portion of the URL, correct? If so, I think that's a fair point and a good catch. I don't have an issue dropping this feature from string-based route values (i.e. '# Route match: $1') in favor of requiring function-based route values for those that need access to the matched string from the URL (as you suggest).

I was thinking that maybe we can rethink how we handle async functions? That is, if a function is sync, we just use its return value as the contents of the page.

This is how it works today. The existing documentation explaining how to write a plugin doesn't make this very clear. This (among other reasons) is why I rewrote those docs which you can see on the develop branch preview:

https://docsify-preview.vercel.app/#/write-a-plugin

How about we do the same with async functions, instead of making you invoke a next function? API seems a little bit more straightforward imo.

No argument from me about the need to revisit out plugin system. How async functions are handled is one of the items on that todo list. The one thing to keep in mind is that the current version of Docsify still supports legacy browsers, changing how async plugins are handled would require a major version bump (I assume), and bumping to the next major version is not as simple as it may seem due to Docsify's history. I'm very much trying to avoid opening up a can of worms and diving into that history here, so apologies if that seems vague.

I think the best approach for now is to separate the functionality proposed in this PR from tasks like rethinking how we handle async plugins, then revisit the plugin system and decide if/how/when we want to move forward there in a separate issue. I'd love to have that discussion if it's something you're interested in.

Hope this helps.

@illBeRoy
Copy link
Contributor Author

illBeRoy commented May 17, 2022

Thanks for the reply! It does help :)

So as far as I understand, our action items are:

  1. Drop support for implicit string formatting (Page: $1)
  2. Keep support for next

Will do.

One more question, though: how do you propose that we determine whether the function is sync or async? since with sync functions we don't wait for anything (just use the returned string), but with async functions we need to await next to be called. Should we assert that the returned value is a promise using duck typing (if it has .next then it's a promise)?

Thanks!

By the way, you can see my draft here (need to add docs and modify the impl to match what we discussed here)

Edit: I see that in Lifecycle, we use function.length to determine whether or not the callback expects a next function (here). So I guess I'll go down that path for now :)

@jhildenbiddle
Copy link
Member

jhildenbiddle commented May 17, 2022

@illBeRoy

I see that in Lifecycle, we use function.length to determine whether or not the callback expects a next function (here). So I guess I'll go down that path for now.

Yep. The plugin system implementation predates any of the current maintainers. I just worked with the existing patterns when possible to reduce risk and avoid requiring a major version bump.

FYI, I also approved the draft so automated tests can run on the PR. You should see the results reflected on Github.

@illBeRoy
Copy link
Contributor Author

Thanks!
Working on the PR as we speak.

@illBeRoy
Copy link
Contributor Author

Hey! PR is ready for review 🙌

@sy-records
Copy link
Member

Hi, @illBeRoy @jhildenbiddle
Can I close this issue? Support completed in #1799?

@trusktr trusktr closed this as completed Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants