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

EXPORT_ES6 mode import.meta.url is not supported #8729

Closed
k2snowman69 opened this issue Jun 2, 2019 · 24 comments · Fixed by #8940
Closed

EXPORT_ES6 mode import.meta.url is not supported #8729

k2snowman69 opened this issue Jun 2, 2019 · 24 comments · Fixed by #8940

Comments

@k2snowman69
Copy link

Issue:
A recent PR resulted in the use of import.meta.url for EXPORT_ES6. However this functionality is currently only at stage 3 (proposal) which means support for this functionality is fairly low which includes eslint, babel, Firefox and Edge all not supporting it currently (all implement at stage 4 or later)

Context:
Looks like there was already a discussion about this and how support isn't quite there yet.
#7137

@VirtualTim
Copy link
Collaborator

Whoops, that was my fault. Originally I was using performance.getEntries().slice(-1)[0].name, which is probably more supported (if much uglier). #8306 (comment)

An idea might be to use performance.getEntries().slice(-1)[0].name, and leave a comment to swap this with import.meta.url in the future. But I'm far from a JavaScript expert, so someone else might have a better idea.

@k2snowman69
Copy link
Author

Another option is to check for the existence of import and import.meta else back onto the old code base.

@davidyxu
Copy link

davidyxu commented Jun 4, 2019

Just wanted to chime in on this conversation, the project I'm working on recently ran into this issue where import.meta.url isn't available (we tried to polyfill it through .

I think it might make sense for this into an opt-in flag where people who are targeting an environment where import.meta.url is reliably available can use it.

https://github.com/emscripten-core/emscripten/pull/8306/files#diff-ae40e645f84e893ce5d037b5efc53be6R2849
i.e. replacing this part of the code with:

if shared.Settings.EXPORT_ES6 and shared.SETTINGS_EXPORT_USE_IMPORT_META :
        script_url = "import.meta.url"

@k2snowman69
Copy link
Author

k2snowman69 commented Jun 4, 2019

Wouldn't checking the existence of it also succeed if it's been polyfilled? Meaning less code and settings for the same goal? Or do you mean that there maybe some people who wouldn't want to use import.meta.url even if it's available?

@davidyxu
Copy link

davidyxu commented Jun 4, 2019

If I'm understanding your suggested approach correctly, I think that would end up being more code being delivered to clients since it would need to be a runtime check.

In general I think putting the flexibility into the hands of the emsdk user is better here because there's a lot of variables in the module's running context that will be unknown to emscripten.

For example, some projects might be going through webpack and webworkers, using a loader like the one shown here:
https://github.com/open-wc/open-wc/blob/master/packages/webpack-import-meta-loader/webpack-import-meta-loader.js
Where it'll do a search and replace of import.meta to a small JS object to polyfill it. Now the project will end up with a web worker that is trying to access window in a context where it doesn't exist.

All of these things are workaroundable, but it feels to me like the simplest fix would be to have emscripten default to something reasonable but options to opt-in to more future facing functionality, that way its easy for a user to fix without code changes in emscripten if they run into a weird situation.

@VirtualTim
Copy link
Collaborator

Just wanted to chime in on this conversation, the project I'm working on recently ran into this issue where import.meta.url isn't available (we tried to polyfill it through .

Out of curiosity, what did you use as the polyfill? Something that would work in an ES6 Module and web worker?

@benjamind
Copy link

We tried using this webpack loader - https://www.npmjs.com/package/@open-wc/webpack-import-meta-loader

Which I believe should work fine for normal main thread usage, but does not function on a webworker. Hence the need to compile to ES6 but also not-use the import.meta.

@kripken
Copy link
Member

kripken commented Jun 7, 2019

@VirtualTim

An idea might be to use performance.getEntries().slice(-1)[0].name, and leave a comment to swap this with import.meta.url in the future. But I'm far from a JavaScript expert, so someone else might have a better idea.

That sounds reasonable, since we have multiple reports of people hitting this problem.

@VirtualTim
Copy link
Collaborator

Thinking about this again, perhaps it's better to just just import.meta behind both EXPORT_ES6 and USE_PTHREADS, since EXPORT_ES6 + USE_PTHREADS currently rely on dynamic module loading anyway, which only currently works in Chrome with Experimental Web Platform features flag set.

For the people having issues with this, did the old document.currentScript technique work for you? I thought that wasn't supported in an ES6 module.

@davidyxu
Copy link

Neither technique really worked for us, we're currently working around it with the SINGLE_FILE flag. The problem with the import.meta.url is that it throws an exception, so the rest of the code can't even evaluate.

It would be great if we could get it working without the SINGLE_FILE flag, the suggested change of performance.getEntries().slice(-1)[0].name would help find the wasm file (although it looks like it makes the assumption that the wasm is served as a sibling to the js glue code).

@kripken
Copy link
Member

kripken commented Jun 20, 2019

This sounds fairly urgent to fix since it is a regression - @VirtualTim do you have time to look into it?

@VirtualTim
Copy link
Collaborator

Yeah sorry, I've been busy with other stuff.
Taking another look at it performance.getEntries().slice(-1)[0].name doesn't always seem to work (occasionally returns "first-paint"). But I'm sure I could do something smarter with performance.getEntries() to make it work.

I'm seeing if there's any better alternatives, however import.meta actually seems fairly wildly supported:
Chrome 64 (March 2018), Firefox 62 (September 2018), Babel 7 (August 2018), Safari (can't seem to find release notes, but appears to have need added in the beta in October 2017). I couldn't find any info about Edge (maybe it's there now that's it's chromium based?), and I'm assuming that IE doesn't work with anything.

Webpack is missing it, but you can use this plugin to add support.

So what is the minimum environment that I should be targeting?

@k2snowman69
Copy link
Author

I think for me personally, the webpack plugin that is referenced along with the fact that rollup already merged in their fix should resolve my initial issue (though still need to test that theory).

However the general question for this project of "what is the minimum environment that I should be targeting?" is still a very important question.

  • If you target tc39 stage 3 then there could be features of which have not been implemented by build tools or browsers even (Edge lags behind but hopefully resolved with the chromium update)
  • If you target tc39 stage 4 then you don't get to use the very useful fun features
  • If you target the latest browser and n-2 versions (common for many websites) you find out that Safari really only publishes new major builds every year and added import.meta.url in Safari 11. Since were currently on Safari 12 and we would back compat all the way to 10 which was released in 2016.

For forward compatibility though, creating a useful way to turn on optional features is great. I have personally come to like typescript's lib option which provides a very detailed but organized solution to turning on future features that aren't currently part of the tc39 stage 4 spec.

@k2snowman69
Copy link
Author

k2snowman69 commented Jun 26, 2019

Also, if someone comes here and is curious how to get babel@7 to work with the import.meta.url, it's not supported by default, you have to add a plugin @babel/plugin-syntax-import-meta. This is also true for jest since jest uses babel in the background. To fix this for jest, you'll have to add the babel-jest library and the plugin mentioned above and override the default babel settings jest has baked in.

Honestly after testing the fix here... (IMO) I'm having to add a lot of extra packages as work arounds because even though the browsers support this feature, the tooling doesn't.

@kkimdev
Copy link
Contributor

kkimdev commented Jun 26, 2019

We're also hitting this issue on our web worker C++ code. The problem for us is that https://www.npmjs.com/package/@open-wc/webpack-import-meta-loader plugin converts import.meta to use "window. ..." instead but web worker doesn't have "window. ..." object.

@benjamind
Copy link

Can't we just make this configurable through a flag like EXPORT_USE_IMPORT_META and EXPORT_USE_CURRENT_SCRIPT and default to the old method (use_current_script).

Then everyone can choose the most appropriate output for their situation.

@VirtualTim
Copy link
Collaborator

So if document.currentScript.src works for all environments apart from an ES6 Module Web Worker, perhaps it best to only use import.meta.url in that one case?

Change the check to:

if shared.Settings.EXPORT_ES6 and shared.Settings.USE_PTHREADS:
  script_url = "import.meta.url"
else:
  script_url = "document.currentScript.src"

I didn't think document.currentScript.src worked in an ES6 module, but if people say it does maybe this is the best solution?

@VirtualTim
Copy link
Collaborator

@kkimdev, @k2snowman69, @benjamind
Any thoughts on my previous comment? (this one)

I do want to fix this, but I'm not sure what the best solution would be that works for everyone.
I don't want to add another flag, or a runtime check, would the above solution work for you guys?

If it doesn't, can you list the options out of MODULARIZE, EXPORT_ES6, USE_PTHREADS, import.meta support, that you use in your configuration?

@k2snowman69
Copy link
Author

k2snowman69 commented Jul 3, 2019

My only concern would be that your tying import.meta.url to pthreads which doesn't seem like the correct association as they do seem like two separate features but I'll leave that to the maintainers. Given that disassociation, I personally would go down the route of a USE_IMPORT_META flag or a more general USE_STAGE_3 to reduce the number of flags but again, I'll leave that decision to the maintainers.

Currently the flags I'm using to build are -s MODULARIZE=1 -s SINGLE_FILE=1 -s EXPORT_ES6=1 and I would want the old functionality.
https://github.com/k2snowman69/shared-makefile/blob/65a997cf3f674dedc3bedb4307883a60c97033a1/emcc-rules.mk

@benjamind
Copy link

I'm away on vacation at the moment so can't check our exact flags but I believe we also use -s MODULARIZE=1 -s SINGLE_FILE=1 -s EXPORT_ES6=1 and run the result in both workers and main thread and would want the old method also.

@curiousdannii
Copy link
Contributor

Does _scriptDir even serve any purpose in SINGLE_FILE mode?

@VirtualTim
Copy link
Collaborator

tying import.meta.url to pthreads which doesn't seem like the correct association as they do seem like two separate features

This is because inside a ES6 module pthread (AKA a webworker in strict mode) there's no access to the DOM, so no access to document.currentScript.src.
Previously I thought that this was a ES6 module restriction, so that's why I chose to use import.meta.url in EXPORT_ES6. However according to all the comments here it sounds like I was mistaken, and this restriction only applies to ES6 module workers (EXPORT_ES6 + USE_PTHREADS).

kripken pushed a commit that referenced this issue Jul 19, 2019
A webworker ES6 module doesn't have access to 'document.currentScript'. 'import.meta' was the only way I could see to achieve that functionality.

Fixes #8729.

Since this appears to fix the issue for everyone who commented on #8729, and no one proposed any better ideas, this is my suggestion.

It would be nice to use import.meta everywhere, and browsers seem to have good support, however the tooling appears to be lagging a bit behind.
@k2snowman69
Copy link
Author

For those using the CI tools using trzecieu's docker build, VirtualTim opened an issue over for the docker image. Here's the link if you want it for tracking purposes!
trzecieu/emscripten-docker#46

kripken pushed a commit that referenced this issue Aug 21, 2019
…9234)

This fixes the problem reported by #9233 and by #8729 while fixing the
    regression bug causes by #8940
@tzvc
Copy link

tzvc commented Feb 5, 2020

Hey there! Any update on this?
I'm trying to make my code bundled by webpack run in a web worker but import.meta.url is not working (it get replaced by a getAbsoluteUrl) function that tries to access the window object. Does anyone have a workaround ?

Cheers!

belraquib pushed a commit to belraquib/emscripten that referenced this issue Dec 23, 2020
)

A webworker ES6 module doesn't have access to 'document.currentScript'. 'import.meta' was the only way I could see to achieve that functionality.

Fixes emscripten-core#8729.

Since this appears to fix the issue for everyone who commented on emscripten-core#8729, and no one proposed any better ideas, this is my suggestion.

It would be nice to use import.meta everywhere, and browsers seem to have good support, however the tooling appears to be lagging a bit behind.
belraquib pushed a commit to belraquib/emscripten that referenced this issue Dec 23, 2020
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 a pull request may close this issue.

8 participants