-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[Pthreads] Fix worker.js in ES6 module environments #21041
Conversation
What about my idea of calling the |
That is a separate topic I think. It does not fix the issues here - we do actually need to fix up It might make sense to do for consistency, separately. But it would be a breaking change OTOH. |
(I also verified it causes no harm to rename to |
src/worker.js
Outdated
@@ -32,7 +39,8 @@ if (ENVIRONMENT_IS_NODE) { | |||
require, | |||
Module, | |||
location: { | |||
href: __filename | |||
// __filename is undefined in ES6 modules | |||
href: typeof __filename !== 'undefined' ? __filename : undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is undefined
really valid here? I think it should be import.meta.url
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined
was enough to pass the test but I have no idea how this is used or what it is 😄 Thanks, I'll change it to that.
Out of curiosity, is it possible to test that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, depends if anything in our JS uses location.href
to get script's own address - presumably that's why it was being polyfilled here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder what are the situations where typeof __filename
wouldn't be undefined
in ES6 mode.
src/worker.js
Outdated
@@ -18,6 +18,13 @@ var ENVIRONMENT_IS_NODE = typeof process == 'object' && typeof process.versions | |||
if (ENVIRONMENT_IS_NODE) { | |||
// Create as web-worker-like an environment as we can. | |||
|
|||
// See the parallel code in shell.js. | |||
#if EXPORT_ES6 && ENVIRONMENT_MAY_BE_WEB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about ENVIRONMENT_MAY_BE_WEB
.. surely require
never works on the web does it?
Where does worker.js use other than below in the ENVIRONMENT_MAY_BE_NODE
block?
It we only use require for those two lines below maybe we should replace them with dyanmic imports?
e.g.:
#if EXPORT_ES6
var fs = await import('fs');
var vm = await import('vm');
#else
var fs = require('fs');
var vm = require('vm');
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah in the main JS ENVIRONMENT_MAY_BE_WEB
is only used because there are static imports for when we know that environment is Node.js and no other.
In this case there are no static imports to fall back to, so it should either add them or just use await import
like you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's confusing. The comment this refers to explains more:
Lines 193 to 196 in 7a14e5b
// `require()` is no-op in an ESM module, use `createRequire()` to construct | |
// the require()` function. This is only necessary for multi-environment | |
// builds, `-sENVIRONMENT=node` emits a static import declaration instead. | |
// TODO: Swap all `require()`'s with `import()`'s? |
My understanding of the lines is that in a pure node build we don't emit this anyhow. But in a mixed build we can't do that, and so if we build for node or web but end up running in node, then we get to this location, and need something to put here (since we couldn't put the static thing for a pure node build).
(This is all separate from this PR, of course, as it's preexisting.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out I was wrong, this is not separable. This PR was enough to fix the case of using a package.json
file to force ES6 module format, but without such a file node.js uses the suffix to decide what to run. And this PR adds await import
, which errors in a non-ES6 module.
To fix that the PR now emits .worker.mjs
when EXPORT_ES6
. That is a breaking change unfortunately, which may break all users that copy files to a production environment that use pthreads+EXPORT_ES6
, but I don't see a way around it? Hopefully not many are using EXPORT_ES6
, and probably few are because of issues like this, actually...
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you use EXPORT_ES6
when it makes sense to create worker.mjs. I don't think think its a huge deal that we change this now. Maybe just mention it in the changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, yeah, already added to the changelog.
If this breaking change sgty then let me know if you have any other feedback on the PR, otherwise I think it is done.
Added some trivial test updates, but also non-trivially a fix that we don't need to check for multiple environments here. That was only needed in the |
This file can be an ES6 module if a
package.json
file indicates that allfiles in the directory are.
We need to apply the same tricks as we apply to the main JS file in that
case, with
createRequire
etc.Followup to #20939 and fixes the
package.json
discussion after that PR landed.