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: conflict between core-js and bfs-process #6136

Closed
wants to merge 4 commits into from

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented Sep 14, 2021

What kind of change does this PR introduce?

This PR fixes an issue with core-js and bfs-process conflicting.

We definitely need to have a look at how to prevent all the global scope poluting we (and mainly browser-fs) does though, as these bugs can be prevented pretty easily that way.

Also probably a good idea to revisit the eval code at some point as it also has some weird behaviour with core-js, specifically this line: https://github.com/zloirock/core-js/blob/master/packages/core-js/internals/global.js#L2

Also this one for detecting v8 engine using the nodeJS path: https://github.com/zloirock/core-js/blob/master/packages/core-js/internals/engine-v8-version.js

Closes #5943

@@ -40,6 +40,11 @@ export default function (
delete allGlobals.global;
}

// This is a hack to make core-js work...
if (window.process.versions) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a pretty big hack, see the PR description for better (bigger) solutions/refactors

@lbogdan lbogdan temporarily deployed to pr6136 September 14, 2021 14:20 Inactive
@lbogdan
Copy link
Contributor

lbogdan commented Sep 14, 2021

Build for latest commit d93eae8 is at https://pr6136.build.csb.dev/s/new.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 14, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d93eae8:

Sandbox Source
Notifications Test Configuration

@lbogdan lbogdan temporarily deployed to pr6136 September 14, 2021 14:36 Inactive
Copy link
Member

@CompuIves CompuIves left a comment

Choose a reason for hiding this comment

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

Makes sense, I would perhaps also put in the comment a link to this PR to make sure that we know exactly why it's happening. Very tricky, some things need window.process, other things break with it...

@lbogdan lbogdan temporarily deployed to pr6136 September 15, 2021 09:42 Inactive
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.

core-js >=3.12 throws errors Target is not iterable
3 participants