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

Include new URL("...", import.meta.url) references in the import graph. #2470

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lgarron
Copy link
Contributor

@lgarron lgarron commented Aug 15, 2022

There are important use cases when it is valuable to get the bundled URL of a file, such as for web worker instantiation: #312

This PR allows esbuild to handle the most portable way to instantiate a module web worker (which works natively in browsers/node, and is understood by a lot of other bundlers already):

const workerURL = new URL("./worker.js", import.meta.url);

// Either construct directly:
new Worker(workerURL, { type: "module" });

// Or construct using a trampoline (needed if the worker code is on a different origin than the current page, e.g. on a CDN):
const blob = new Blob([`import "${workerURL}";`], { type: "text/javascript" });
new Worker(URL.createObjectURL(blob), { type: "module" });

This PR implements support by recording such relative URLs in the import graph during AST parsing, thanks in part to @firien here. However, it still has issues:

  • It's currently hardcoded to detect .ts and .js suffixes so I can test it. But:
    • In TypeScript, it should be possible to use import paths without a file extension.
    • Import paths should work, e.g. @some/package/worker if @some/package exports ./worker.
    • Other file types (e.g. .tsx and .jsx) should be included by default, possibly any file types as long as the file exists at the referenced relative path (to fully address URL assets bundling #795).
  • Given that this relies on import.meta.url, it should probably be disabled (or even error) for CJS output.
  • Is it okay to hardcode detection for new URL as opposed to looking for URL constructors in other ways?
  • I haven't added tests yet.

@evanw Could I ask if you think the approach in this PR will work, and the best way to address the issues listed above?

@lgarron
Copy link
Contributor Author

lgarron commented Aug 23, 2022

  • I haven't added tests yet.

I've added a test that covers the most important behaviour. Hopefully I've added it in the appropriate place!
I could still use advice on the other points.

@lgarron
Copy link
Contributor Author

lgarron commented Aug 30, 2022

@evanw Anything I can do to help move this forward?

This implementation matches what I believe you support as the best approach: #312 (comment)

@evanw
Copy link
Owner

evanw commented Aug 31, 2022

I'm planning on releasing a form of this sometime soon. You can see my work in progress version here: #2508. This will initially require code splitting to be enabled (as well as bundling and ESM output of course) as esbuild's internals currently mean each input module only shows up in one output file. It will also require paths to be explicitly relative (i.e. start with ./ or ../) to avoid ambiguity. Otherwise it will work very similar to how import() already works. So for example if you do new URL('./foo.bar', import.meta.url) and .bar files are loaded with the copy loader then it will be copied to the output directory. But if .bar files are loaded with the json loader then it will be parsed as JSON and converted to a JavaScript file with that JSON data as the default export.

  • Import paths should work, e.g. @some/package/worker if @some/package exports ./worker.

This doesn't seem like something that should be done because new URL('foo/bar', 'http://xyz.com/abc/def') is the same as new URL('./foo/bar', 'http://xyz.com/abc/def') and results in the URL http://xyz.com/abc/foo/bar. It seems strange to me that esbuild would intercept this syntax and interpret it in some way that's not a relative URL when it looks like it would behave as one.

@lgarron
Copy link
Contributor Author

lgarron commented Aug 31, 2022

'm planning on releasing a form of this sometime soon. You can see my work in progress version here: #2508.

Ooooh, thanks, I'm really glad to hear it! 😍

This will initially require code splitting to be enabled (as well as bundling and ESM output of course) as esbuild's internals currently mean each input module only shows up in one output file. It will also require paths to be explicitly relative (i.e. start with ./ or ../) to avoid ambiguity. Otherwise it will work very similar to how import() already works. So for example if you do new URL('./foo.bar', import.meta.url) and .bar files are loaded with the copy loader then it will be copied to the output directory. But if .bar files are loaded with the json loader then it will be parsed as JSON and converted to a JavaScript file with that JSON data as the default export.

That sounds pretty good to me!

Does this also meant that new URL('./file', import.meta.url) will pick up a relative ./file.ts?
I just tried to test, but I'll comment on the PR with my issues.

This doesn't seem like something that should be done because new URL('foo/bar', 'http://xyz.com/abc/def') is the same as new URL('./foo/bar', 'http://xyz.com/abc/def') and results in the URL http://xyz.com/abc/foo/bar. It seems strange to me that esbuild would intercept this syntax and interpret it in some way that's not a relative URL when it looks like it would behave as one.

That seems fair. I don't personally need this, but I do know some others will need this use case. I think it could usually be handled by a relative import of a simple file that wraps the module import.

@lgarron
Copy link
Contributor Author

lgarron commented Aug 31, 2022

Does this also meant that new URL('./file', import.meta.url) will pick up a relative ./file.ts?
I just tried to test, but I'll comment on the PR with my issues.

Okay, I was able to test by running a production build instead of watch mode.
Looks like it works! :-D

@evanw evanw mentioned this pull request Aug 31, 2022
lgarron added a commit to lgarron/esbuild that referenced this pull request Jan 24, 2023
`make js-api-tests` fails to include the worker file in the build.

This is an adaptation of evanw#2470 , for evanw#2866
lgarron added a commit to lgarron/esbuild that referenced this pull request Oct 13, 2023
`make js-api-tests` fails to include the worker file in the build.

This is an adaptation of evanw#2470 , for evanw#2866
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.

3 participants