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

Refactor handling of import.meta.url and add option to configure behaviour #2785

Merged
merged 10 commits into from Apr 11, 2019

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Apr 1, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no (could be discussed, though)

List any relevant issue numbers:
Resolves #2748

Description

This will refactor the current default handling of import.meta.url in several important ways:

  • There is no specific "compact mode" as that was essentially untested and added complexity. If there is demand, this could be re-added later but my assumption is that as this is not wrapper code, compact mode is not that important for this feature.
  • The generated code has been refined together with some assumptions:
    • all formats except cjs and umd assume the code runs in a browser environment where URL and document are available
    • for cjs and umd, presence of a browser environment is detected by checking for document (after which presence of URL is assumed)
    • fallbacks will now take the generated chunk name into account for some formats in the form new URL('my/bundle.js', document.baseURI).href, but this is only the last option if e.g. document.currentScript.src is not available.

Generated code:

// amd
new URL(module.uri, document.baseURI).href

// cjs
(typeof document === 'undefined' ? new (require('u' + 'rl').URL)('file:' + __filename).href : (document.currentScript && document.currentScript.src || new URL('bundle.js', document.baseURI).href))

// esm (unmodified)
import.meta.url

// iife
(document.currentScript && document.currentScript.src || new URL('iife.js', document.baseURI).href)

// system
module.meta.url

// umd (same as cjs)
(typeof document === 'undefined' ? new (require('u' + 'rl').URL)('file:' + __filename).href : (document.currentScript && document.currentScript.src || new URL('umd.js', document.baseURI).href))

Also by using the new resolveImportMetaUrl plugin hook, it is now possible to customize this behaviour to e.g. always resolve using the original module paths. Example (I also put this into the docs):

// rollup.config.js
import path from 'path';

export default {
  // ...,
  plugins: [{
    // I used an object for the hook. For now, it contains moduleId and chunkId
    // but could receive additional properties in the future
    resolveImportMeta(prop, {moduleId}) {
      const url = `new URL('${path.relative(process.cwd(), moduleId)}', document.baseURI).href`;
      if (prop === 'url') {
        return url;
      }

      // also handle just `import.meta`
      if (prop === null) {
        return `{url}`;
      }

      // use the default behaviour for all other props
      return null;
    }
  }]
};

@coreyfarrell
Copy link

I've posted rollup-test which demonstrates how I could use the importMetaUrl function to produce the proper URL for ESM output. To my knowledge import.meta.url is expected to be a fully qualified URL (including protocal://hostname/). For the sake of discussion lets say that bundle.js from rollup-test is served at http://localhost/bundle.js, I expect that script to write two messages to the console:

http://localhost/dir1/index.js
http://localhost/dir2/index.js

Obviously this is a silly example but the whole point is that if dir1/index.js uses import.meta.url, it will perform manipulations to that value to find something relative to dir1/index.js.

Unfortunately an importMetaUrl function cannot be shared between different formats as there is no universal way to determine the absolute URL of the script. It might be helpful if there were a function that was expected to return the URL of moduleId relative to the URL of the output file. So for the rollup-test repo I'd return path.posix.relative(process.cwd(), moduleId). Then rollup would use the output format to decide how to wrap that relative URL.

My basic idea:

const bundleURL = {
  amd: 'new URL(module.uri, document.baseURI).href',
  esm: 'import.meta.url',
  // all other formats as you declared above..
};

function getImportMetaReplacementURL(output, chunkId, moduleId) {
  if (output.relativeMeta) {
    const relativeURL = JSON.stringify(output.relativeMeta(chunkId, moduleId));
    // simplified example omits the `require('url')` needed for some formats to get URL
    return `new URL(${relativeURL}, ${bundleURL[output.format]}).href`;
  }

  if (output.importMetaUrl) {
    return output.importMetaURL(chunkId, moduleId);
  }

  return bundeURL[output.format];
}

In theory relativeMeta could have a default implementation path.posix.relative(chunkId, moduleId) but this would be a breaking change for users of babel-plugin-bundled-import-meta or anything else where import.meta.url is already processed by a plugin.

Sorry this response got so long, thanks for working on this.

@LarsDenBakker
Copy link
Contributor

I think this a good solution, and we can iterate over different solutions for setting the final bundled import.meta.url.

@lukastaegert lukastaegert force-pushed the refactor-import-meta branch 3 times, most recently from 81a8fc1 to ef4109e Compare April 5, 2019 14:31
@lukastaegert
Copy link
Member Author

Following communication with @guybedford, I changed the implementation to use a plugin hook instead of an output option. This has the advantage that this can now easily be packed into a plugin that e.g. also does asset handling (otherwise there might be conflicts between plugins and options).

The hook is synchronous and all usages of import.meta.url are passed to this hook. Returning null will default to the builtin behaviour. I also added an example to the docs how the hook could be used to resolve relative to the original filenames.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, this looks really powerful now!

@justinfagnani I'm sure you'll be very pleased to see this finally.

@justinfagnani
Copy link

@guybedford thanks for the tag.

What changed? We haven't been able to use Rollup's import.meta support before, so we've compiled it out before source hit Rollup.

The big things we need are

  1. import.meta.url should always resolve to the original module's URL. Seems like we could use resolveImportMetaUrl now, but that hook in the example should be the default behavior. It's the only way that import.meta.url will work both unbundled and bundled, say with something like fetch(new URL('./styles.css', import.meta.url)). The iife output above looks wrong as that will be the URL of the bundle. That would almost never have been the URL of the module. I'm unsure of the utility of a hook as the spec is very clear on what import.meta.url resolves to, and I don't see the point in changing it. The configuration option most needed (but still usually unnecessary) is to override the root path that module paths are resolved against, in case bundles are moved after building.

  2. Support for passing import.meta as an object. This means that import.meta needs to be compiled to something like {...import.meta, url: new URL('./module-path-relative-to-document.js').href}.

Glad there's work here. We can try this out soon I think. Along with #2772 we might be able to replace Polymer Bundler with a Rollup plugin.

@lukastaegert
Copy link
Member Author

Changing the default could certainly be possible in a new major version but there are some concerns I have about it. Especially the fact that there might be no "naturally correct" URL to resolve to. Example:

There are two entry points in ~/my-project/src/entry1.js and ~/my-project/lib/entry2.js. Both are deployed to the server on the same as http://server.net/chunk1 and http://server.net/chunk2. What should the correct value of import.meta.url be in both of those files and why? What about files that are imported from a node_modules folder? My feeling is that there might be some hidden assumptions in your proposal that may not be universally true.

I certainly see that we are taking the easy way out here by saying: Not transpiling import.meta.url for esm is Rollup's default and we translate all other formats accordingly but I honestly do not know what a universally agreed "correct" alternative behaviour should be. And I do not think anyone would want to have all their private local paths compiled into the bundle. Still, defaults can be changed in major versions if there is strong demand.

About the object form: What is your use-case here? I was thinking about extending the hook to handle arbitrary import.meta properties but you actually want something else?

@lukastaegert
Copy link
Member Author

Please also note that process.cwd() is nothing that Rollup core has access to. Rollup core does not know much about the environment it runs it so everything needs to be passed as options.

@justinfagnani
Copy link

Especially the fact that there might be no "naturally correct" URL to resolve to.

There very much is though: load the project without bundling. Whatever each individual module's import.meta object's url property is is what it should be after bundling. Bundling should not change the behavior of the program.

Example:

There are two entry points in ~/my-project/src/entry1.js and ~/my-project/lib/entry2.js. Both are deployed to the server on the same as http://server.net/chunk1 and http://server.net/chunk2. What should the correct value of import.meta.url be in both of those files and why?

The URLs should be the relative patch between the chunk and the bundlined module, with a base of the chunk, which should be the original URL of the module. So if the chunk is output to ~/my-project/chunk1.js, then in entry1.js, import.meta should be rewritten to {...import.meta, url: new URL('./src/entry1.js', import.meta.url)}. This preserves the original URL of entry1.js relative to the chunk it was bundled into.

What about files that are imported from a node_modules folder?

No difference. Their import.meta.url should resolve to the URL of the file without bundling. It's especially import there if third party modules want to load resources out of their own package.

My feeling is that there might be some hidden assumptions in your proposal that may not be universally true.

I think the only assumption is that the program should work the same as before bundling. Prior to bundling the semantics are very clear.

About the object form: What is your use-case here? I was thinking about extending the hook to handle arbitrary import.meta properties but you actually want something else?

In Polymer we allow passing the meta object into a base class that does path manipulation on CSS to correctly load relative resources. We accept the meta object just as convenience, and in case in the future there were other meta properties that the base class might need. It looks like this:

class A extends PolymerElement {
  static get importMeta() { return import.meta; }
}

The base class can then access this.constructor.importMeta.url, and get the subclass module's URL, even though the base class is defined in a different module.

That's why we rewrite import.meta to {...import.meta, url: new URL(pathFromChunkToModule, import.meta.url)} instead of rewriting import.meta.url.

@justinfagnani
Copy link

Another way to derive the correct rewrite is that in a module import('./foo.js') and fetch(new URL('./foo.js', import.meta.url)) should always load the same file.

@lukastaegert
Copy link
Member Author

lukastaegert commented Apr 8, 2019

I see. So what you want is that if you serve your complete project folder including all sources and compiled output files, the behaviour should be the same. This is definitely useful but I am not sure that serving all their sources is everyones use-case. Another way of using it could be to copy files next to the processed chunks which would work well with the current state of things.

For the time being, I would stick with this PR as it is a non-breaking change, but we can consider changing it for the next major. In any case, having a hook to configure it seems to be very helpful.

About replacing import.meta: Also doable. I'm not 100% happy about using a spread operator as it is rather new syntax but this could pan out over time. Still, NOT replacing should still be easily possible if not the default.

As "optimal" defaults are really very much subjective, how about generalizing the plugin hook in the following way:

resolveImportMeta: (prop: string | null, {moduleId: string, chunkId: string}) => string | null

prop would be either the name of a prop, e.g. url for import.meta.url, or null if import.meta is used without any prop. Returning null would preserve import.meta.whatEver for esm output or do something sensible for other formats (like the current default for import.meta.url, undefined for other props and an object containing url if prop is null)

@lukastaegert lukastaegert mentioned this pull request Apr 8, 2019
9 tasks
@lukastaegert
Copy link
Member Author

lukastaegert commented Apr 8, 2019

Another thing to consider with your proposal is how to handle files "outside" the base folder. For instance when bundling to ~/my-project/dist, what code should be generated in ~/my-project/src/index.js? Or even ~/my-project/main.js? Or folders outside my-project (like a shared node_modules folder)?
I see no easy answers here that will not break when served. And bundling to a sub-folder seems to be a rather common use-case.

@coreyfarrell
Copy link

About replacing import.meta: Also doable. I'm not 100% happy about using a spread operator as it is rather new syntax but this could pan out over time. Still, NOT replacing should still be easily possible if not the default.

I think this is not an issue. Object spread is stage 4, import.meta is still stage 3. MDN also shows that object spread was implemented in major browsers before import.meta.

For the time being, I would stick with this PR as it is a non-breaking change, but we can consider changing it for the next major.

I appreciate this. Any change to the current default handling of import.meta.url by rollup will break my builds. Granted I could add resolveImportMeta: null to get the existing default behavior but that's something I shouldn't have to do for an in-range update to rollup (completely reasonable when I upgrade to rollup@^2.0.0).

@lukastaegert
Copy link
Member Author

MDN also shows that object spread was implemented in major browsers before import.meta

Good point!

@lukastaegert
Copy link
Member Author

I changed the hook to handle ALL import.meta props and free occurrences of import.meta, please have another look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preserve import.meta.url from before bundling
5 participants