Skip to content

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

Merged
nicolo-ribaudo merged 2 commits intobabel:masterfrom
nicolo-ribaudo:fix-resolve-standalone
Apr 16, 2020
Merged

Don't include "resolve" in @babel/standalone#11432
nicolo-ribaudo merged 2 commits intobabel:masterfrom
nicolo-ribaudo:fix-resolve-standalone

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 🤔

"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.

3 participants