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

Remove return outside of function as it is not es6 module compliant #3

Merged
merged 1 commit into from
Dec 17, 2019

Conversation

FranzPoize
Copy link

Since which-collection has been added to is-equal there's a lot of module dependant on is-map. According to babel/babel#1298 returns outside of function are not es6 module compliant. So I submit this PR to avoid the return.

I changed the if to avoid the negative condition. Please give me some feedback if you're unhappy with anything. Thank you.

@ljharb
Copy link
Member

ljharb commented Dec 3, 2019

This file isn’t an es6 module, it’s a CommonJS module, which it is compliant with. You shouldn’t be transpiling code you didn’t write, which includes everything in node_modules, and this package.

@pocka
Copy link

pocka commented Dec 16, 2019

This file isn’t an es6 module, it’s a CommonJS module, which it is compliant with. You shouldn’t be transpiling code you didn’t write, which includes everything in node_modules, and this package.

I agree with we should not transpile dependencies. If you need to do it (ex. creating a bundle for ES5 environment), you should use options like allowReturnOutsideFunction instead.

But let me correct a few. CJS spec does not mention to top-level return. It works because "Node.js runs Node.js modules (≒ CJS modules) with wrapper code". And, IIRC, both ES5 and ES6 language spec don't allow top-level return statements regardless of Scripts or Modules. For reference, this issue might help (too old but I think it makes sense).

@ljharb
Copy link
Member

ljharb commented Dec 16, 2019

The legacy CJS spec is irrelevant; “what node supports” is what node modules require, and top level return is part of that.

node modules are not a Script, they’re a function scope - and as such, top level return is already valid. Any tool that claims to support node modules but doesn’t support top level return is lying or broken.

@pocka
Copy link

pocka commented Dec 16, 2019

@ljharb
Yeah, my point was that some people (especially run JS on browser with bundlers) consider CJS as "legacy CJS spec", so it's safer to say "node modules" or something. Sorry for confusing writing 😰

@ljharb
Copy link
Member

ljharb commented Dec 16, 2019

The only module types that matter (or have for years) are node modules (commonly called CJS) or native language modules (called ESM).

@ljharb ljharb closed this Dec 16, 2019
@ljharb ljharb added invalid This doesn't seem right wontfix This will not be worked on labels Dec 16, 2019
@FranzPoize
Copy link
Author

This was an enjoyable experience thank you.

@ljharb
Copy link
Member

ljharb commented Dec 17, 2019

Turns out that this is caused by multiple kinds of breakage:

  1. webpack, prior to v4.31, does not properly parse CJS modules
  2. babel does not properly parse CJS modules without the allowTopLevelReturn option enabled
  3. CRA unsafely transpiles node_modules, and does not enable the allowTopLevelReturn babel option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants