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

Don't include "resolve" in @babel/standalone #11432

Merged
merged 2 commits into from Apr 16, 2020

Conversation

nicolo-ribaudo
Copy link
Member

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Starting from browserify/resolve#217, resolve unconditionally uses the fs object (even if we don't call any resolve function).

This is a problem in @babel/standalone since fs isn't available there. However, resolve shouldn't have been included in @babel/standalone in the first place 🤷

@existentialism
Copy link
Member

Is there any way to add a test for this?

@nicolo-ribaudo
Copy link
Member Author

I tried to search for a rollup option to disallow some modules, but I couldn't find it.

@nicolo-ribaudo
Copy link
Member Author

It is still including resolve on windows 🤔

@@ -11,6 +11,10 @@
"keywords": [
"babel-plugin"
],
"browser": {
Copy link
Contributor

@JLHwung JLHwung Apr 16, 2020

Choose a reason for hiding this comment

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

Seems like it suffices to set preferBuiltins: false when building babel-standalone, so it can warn us on builtin imports which is not yet covered by rollup-plugin-node-polyfill.

Copy link
Contributor

@JLHwung JLHwung Apr 16, 2020

Choose a reason for hiding this comment

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

Oh but it will not cover cases like resolve is imported. Maybe we can do something like

{
  name: "warn-browser-unsupported-modules",
  resolveId(...args) {
    const [source] = args;
    if (["resolve"].includes(source)) {
      throw new Error(`${source} can not be bundled into @babel/standalone!`);
    }
    return this.resolve(...args);
  }
}

Anyway I am happy if we merge it as-is and postpone to another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm merging this for now and I will investigate. I'm surprised that I can't find a plugin that does what we need 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicolo-ribaudo You may checkout https://github.com/dumbmatter/rollup-plugin-blacklist but I am not strongly for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

May I ask why don't you like it?

@nicolo-ribaudo nicolo-ribaudo merged commit d9eb943 into babel:master Apr 16, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the fix-resolve-standalone branch April 16, 2020 18:19
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 17, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: standalone PR: Fixes failing main
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants