Standardize mapping of browser URLs to file paths (for Resource plugins)

[thescientist13]

Overview

As has come up in a couple recent issues which I believe are part a result of the fallout from #520 (changed the ordering of how default plugins run)

• strip query params from URLs #579
• fetching any JSON file that is not graph.json always returns an ES module #604
• dev server proxy is not forwarding query params #598
• exclude node modules from user workspace resolution #687
• query string parameters in node_modules (CSS) paths returns no content and 204 response #690

We have a little bit of a conundrum in this particular section of code which does the bulk of the work to resolve URLs in the browser to actual files on disk for the following (in this order)

• node_modules
• user workspace
• any custom resolvers from a Resource plugin from the user (FI runs first)

app.use(async (ctx, next) => {
  ctx.url = await resources.reduce(async (responsePromise, resource) => {
    const response = await responsePromise;
    const { url } = ctx;
    const resourceShouldResolveUrl = await resource.shouldResolve(url);
    
    return resourceShouldResolveUrl
      ? resource.resolve(url)
      : Promise.resolve(response);
  }, Promise.resolve(ctx.url));

  // bit of a hack to get these two bugs to play well together
  // https://github.com/ProjectEvergreen/greenwood/issues/598
  // https://github.com/ProjectEvergreen/greenwood/issues/604
  ctx.request.headers.originalUrl = ctx.originalUrl;

  await next();
});

Basically given the following types of URLs

• something.css?type=css
• /api/events?query=something

The issues boil down to

• query strings will break file system access / logic, eg. fs.readFile('something.css?type=css)
• If a file exists in both node_modules and user workspace, there can be incorrect results when trying to map relative resources

Considerations

Query Params

• strip query params from URLs #579
• fetching any JSON file that is not graph.json always returns an ES module #604
• dev server proxy is not forwarding query params #598

const { url } = ctx; // goes to resolvers

ctx.request.headers.originalUrl = ctx.originalUrl; # stashed in a header for plugins - https://github.com/ProjectEvergreen/greenwood/blob/v0.15.0/packages/plugin-import-css/src/index.js#L22

Because of needing to support query params for API calls or appending hints for non-standard ESM, we have two options

1. Force every resolver to use barePathUrl to strip query params
2. Strip it at the Koa level before handing to resolvers and still keep in a secret header (current)

Although solution 2 has been working, it's a bit of a "hidden" API, so not sure if there is enough docs for it, or a better way to handle this. But it avoid downstream plugins from breaking because of the file reading issue.

Resolvers

• exclude node modules from user workspace resolution #687
• query string parameters in node_modules (CSS) paths returns no content and 204 response #690

After Greenwood resolvers go first, the results of those never get passed to next plugins, because we always pass the original URL to each resolvers, instead of saying forwarding the results of the node_modules resolver to any other resolvers that may want to reuse that logic.

  ctx.url = await resources.reduce(async (responsePromise, resource) => {
    const response = await responsePromise;
    // const { url } = ctx; just keep using the resolved response
    const resourceShouldResolveUrl = await resource.shouldResolve(response);
    
    return resourceShouldResolveUrl
      ? resource.resolve(response)
      : Promise.resolve(response);
  }, Promise.resolve(ctx.url));

There is a referenced issue to just forward the resolved URL, which seems fine for now. But there is another issue that to support relative URLs, which helps with things like #689 (comment), we have to make sure that (for example), user workspace resolver ONLY looks in the user workspaceinstead of trying to a node_modules path and reducing it down to something that might actually be in the user's workspace, by coincidence.

Not sure if we need to something like a negative resolve check to make sure user workspace isn't going to inadvertently resolve to some other legitimate resource? Otherwise, we can't always deny all possibilities, like we can since we know about node_modules

[thescientist13]

Could maybe help and look into handling relative paths for theme packs?
#689 (comment)

[thescientist13]

Also wonder if plugin-user-workspace should not assume a local workspace match and be more like plugin-node-modules?

So instead of

const bareUrl = this.getBareUrlPath(url);
const workspaceUrl = fs.existsSync(path.join(userWorkspace, bareUrl))
  ? path.join(userWorkspace, bareUrl)
  : path.join(userWorkspace, this.resolveRelativeUrl(userWorkspace, bareUrl));

It would be

const bareUrl = this.getBareUrlPath(url);
const workspaceUrl = fs.existsSync(path.join(userWorkspace, bareUrl))
  ? path.join(userWorkspace, bareUrl)
  : this.resolveRelativeUrl(userWorkspace, bareUrl)
    ? path.join(userWorkspace, this.resolveRelativeUrl(userWorkspace, bareUrl))
    : bareUrl;

[thescientist13]

This also relates to I think to some of the rough edges encountered so far with Theme Packs, in terms of being able to straddle development vs consumption of a theme pack plugin, all while using Greenwood - #682 (comment)

[thescientist13]

Another thing to keep in mind is that if we are recursively walking these directories to resolve nested paths, then we might end up giving a false sense of assurance when resolving files in development, that would break for Rollup since the paths are not "exactly" correct.

For example, take this structure

src/
  components/
     posts-lists/
       posts-lists.js
  services/
    posts-service.js

In post-lists.js you could do this , which would be incorrect 🚫

import { postsService } from '../services/posts-service.js';

instead you need an additional ../. ✅ '

import { postsService } from '../../services/posts-service.js';

So while this is fine during development with our resolve convenience, the first example will break though for Rollup

So while it is convenient, it does have to be "right", since if it just breaks in Rollup, it would be better to break in development, too.

[thescientist13]

Maybe I am just completely over thinking it? Maybe instead of recursive approach, just assume a starting directory (user's workspace) and resolve everything from there only?
https://www.reddit.com/r/webdev/comments/puw8tf/relative_path_names_getting_truncated_by_the/

[thescientist13]

Another observation, around query string handling.

As alluded to in the description of this discussion, mapping URLs to files mostly works, but things like query strings (styles.css?v=1) on URLs will "break" logic like path.extname which has been the cause of another bug - #765 (again).

Perhaps running this.getBarePathUrl should be done at the resource level automatically? Seems like it would get annoying if every Resource plugin author had to do this, and there is always originalUrl if needed.

[thescientist13]

Another case to handle / consider here is implicit support for a Not Found page. Currently, you can navigate to it directly bu visiting /404.html in your browser. In production, we highlight that a web server can handle that fallback handling.

However, if you try and visit an actual missing page during development or when running serve, there is no handling for a not found page

Development
Shows the default page template it seems and no errors in the terminal

Serving
Has a legitimate 404 error

---

Would like to make this explicit, maybe through a config flag? Mostly just want to keep the boundaries between dev and prod clean, and try to be super un-opinionated in the server behaviors out of the box.

[thescientist13]

Found this, document.currentScript, not sure if it would help at all? Seems like not for modules though?

[thescientist13]

Created an issue to take this on - #949