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

Fix for MessageChannel in Node Envs (#20756) #20790

Closed
wants to merge 10 commits into from
Closed

Fix for MessageChannel in Node Envs (#20756) #20790

wants to merge 10 commits into from

Conversation

amensum
Copy link

@amensum amensum commented Feb 10, 2021

Please, review my PR for fix issue #20756

Copy link
Collaborator

@gaearon gaearon left a 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".

@amensum
Copy link
Author

amensum commented Feb 10, 2021

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

@sizebot
Copy link

sizebot commented Feb 10, 2021

Comparing: 3d10eca...cda9108

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 122.54 kB 122.54 kB = 39.47 kB 39.46 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.11 kB 129.11 kB = 41.51 kB 41.51 kB
facebook-www/ReactDOM-prod.classic.js = 406.66 kB 406.66 kB = 75.39 kB 75.39 kB
facebook-www/ReactDOM-prod.modern.js = 395.01 kB 395.01 kB = 73.50 kB 73.50 kB
facebook-www/ReactDOMForked-prod.classic.js = 407.00 kB 407.00 kB = 75.44 kB 75.44 kB
oss-experimental/scheduler/index.js +126.89% 0.33 kB 0.75 kB +84.39% 0.21 kB 0.38 kB
oss-stable/scheduler/index.js +126.89% 0.33 kB 0.75 kB +84.39% 0.21 kB 0.38 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/scheduler/index.js +126.89% 0.33 kB 0.75 kB +84.39% 0.21 kB 0.38 kB
oss-stable/scheduler/index.js +126.89% 0.33 kB 0.75 kB +84.39% 0.21 kB 0.38 kB

Generated by 🚫 dangerJS against cda9108

@amensum amensum requested a review from gaearon February 10, 2021 21:51
Copy link
Author

@amensum amensum left a 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

typeof window.document.createElement !== 'undefined'
);

// Checks if this is global space
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

Copy link
Author

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.

Copy link
Collaborator

@gaearon gaearon Feb 12, 2021

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?

Copy link
Author

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.

Copy link
Author

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

@gaearon
Copy link
Collaborator

gaearon commented Feb 17, 2021

Thank you for the PR! After more consideration we've landed on a different strategy: #20834

Appreciate your exploration!

@gaearon gaearon closed this Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants