-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Fix for MessageChannel in Node Envs (#20756) #20790
Conversation
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 don't think this would work as we want. E.g. it would probably detect React Native as "DOM". While a webpack config that has a process
shim would be detected as "no DOM".
I got it, i will change it |
Comparing: 3d10eca...cda9108 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
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.
Added commit that removes incorrect logic with process
packages/scheduler/npm/index.js
Outdated
typeof window.document.createElement !== 'undefined' | ||
); | ||
|
||
// Checks if this is global space |
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.
Why?
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.
To catch execution in node environments and use 'unstable_no_dom'.
expression | node | chrome | react native |
---|---|---|---|
globalThis === this | false | true | false |
Sorry if I'm wrong somewhere... I added a more detailed comment to the line.
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 understand your goal, I'm asking what makes this comparison the right one to use from your perspective. Can you explain the reasoning, aside from the fact that it seems to work in your testing?
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 rely on the fact that in the top-level code in a node module, this
is equivalent to module.exports
and globalThis
is equivalent to global
. Whereas in the top-level code in a browser, this
and globalThis
is equivalents to window
.
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 had an assumption that it is necessary to use a condition like:
typeof window !== 'undefined' &&
typeof globalThis !== 'undefined' &&
globalThis === window
Thank you for the PR! After more consideration we've landed on a different strategy: #20834 Appreciate your exploration! |
Please, review my PR for fix issue #20756