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

Should export ES5-compatible for npm, otherwise babel transform is required externally #139

Closed
sompylasar opened this issue Nov 15, 2015 · 21 comments
Assignees

Comments

@sompylasar
Copy link

The ES2015-rewritten 6.0.0 has issues when using via webpack. Usually, babel transform is only applied to the app code, and the node_modules directory is excluded entirely. This does not work with 6.0.0 compared to ES5-compatible 5.2.0 -- webpack bundles the ES2015 code of the node_modules/qs right into the output, which then breaks the uglify step.

Please provide ES5-compatible build without browserify and point package.json's main to it.

This can be done in the prepublish step. Look how it's implemented.

Related: #128, #129, #108.

@hueniverse
Copy link
Contributor

Just use the previous version for now. They are the same.

@sompylasar
Copy link
Author

@hueniverse yes, I do this for now.

@bdefore
Copy link

bdefore commented Nov 16, 2015

i ran into this issue while updating my dependencies and saw the latest was 6.0.0 ... unfortunately this caused me to see the error SyntaxError: let is a reserved identifier and it was due to the issue @sompylasar describes. this should probably not be published in npm right now.

@hueniverse
Copy link
Contributor

I suggest you learn how semver works and don't upgrade to a new major version without understanding what broke.

@bdefore
Copy link

bdefore commented Nov 16, 2015

@hueniverse i am aware of how semver works, thank you. thought you might want to know that there's more than one person that would like to use qs latest with their other ES5 transpiled libraries.

@arb
Copy link
Contributor

arb commented Nov 17, 2015

You can also add the path to qs in your array of included files for the babel transformer. Then you could exclude all of the other node_modules, but include qs.

@zerkms
Copy link

zerkms commented Nov 19, 2015

@arb

You can also add the path to qs in your array of included files for the babel transformer

how would one do that actually? If node_modules is excluded using exclude parameter - you cannot include anything from it.

@arb
Copy link
Contributor

arb commented Nov 19, 2015

When you are setting up the loader, if you are excluding "node_modules", there is also an include option where you can pass an array of values. Add the path to QS into that array. (This is just speculating as I haven't done this, but I think that should work.)

module: {
  loaders: [
    {
      test: /\.js?$/,
      exclude: /node_modules/,
      include: [
        'path/to/your/app',
        'node_modules/qs'
      ]
      loader: 'babel',
      query: {
        presets: ['es2015']
      }
    }
  ]
}

Something along those lines. You'll have to play around with the include paths but that is the general gist of it.

@sompylasar
Copy link
Author

sompylasar commented Nov 19, 2015 via email

@nlf nlf self-assigned this Nov 19, 2015
@nlf nlf added the non-issue label Nov 19, 2015
@nlf
Copy link
Collaborator

nlf commented Nov 19, 2015

There is no way to resolve this because there is no issue. This module is ES6 now. If you need ES5, either use an older version or configure your transpiler to deal with it for you.

@nlf nlf closed this as completed Nov 19, 2015
@sompylasar
Copy link
Author

sompylasar commented Nov 19, 2015 via email

@sompylasar
Copy link
Author

sompylasar commented Nov 19, 2015 via email

@nlf
Copy link
Collaborator

nlf commented Nov 19, 2015

I'm aware of the many methods available for maintaining an ES5 version of this module. I have no interest in doing so. If you really want an ES5 version, build and publish one. I'm happy to link to it from here.

@sompylasar
Copy link
Author

sompylasar commented Nov 19, 2015 via email

@zerkms
Copy link

zerkms commented Nov 19, 2015

@arb that was my exact point: if a path is in exclude - there is no way to make it loaded. That's what exclude is for - to exclude.

(This is just speculating as I haven't done this, but I think that should work.)

It won't.

@Marsup
Copy link
Contributor

Marsup commented Nov 19, 2015

Come on, it's a regex, It's not that hard to exclude /node_modules\/(?!qs)/...

I stand with @nlf, it's your problem if you want to take it into an ES5-only environment, this module explicitly says it uses node 4+ and YOU have ways to deal with it.

@dantman
Copy link

dantman commented Dec 4, 2015

I'd like to state that in some cases "configure your transpiler to deal with it for you" is actually a ridiculous suggestion.

I'm using browserify, but:

  • qs hasn't declared something like "browserify": {"transform": [["babelify", {"presets": ["es2015"]}]]} so browserify doesn't work out of the box and I can't add this to the qs package myself.
  • "linking" to a 3rd party published browser build won't change anything since browserify wouldn't pick up on it, it's not actually part of the module.
  • The "other" option of configuring babelify as a global transform, doesn't actually work. Or rather, whether it works or not depends entirely on every other random module you depend on.
    • When you ignore files, that doesn't actually tell the transform to go and ignore entire modules. It's a babel level option and subject to other babel level options defined elsewhere. So babel still goes through the tree and parses other options like those in .babelrc files belonging to every single module you depend on.
    • This sort of works... up until you hit one of the modules that is transpiled during prepublish using babel@5, and then this happens Unhandled rejection ReferenceError: [BABEL] /.../node_modules/react-leaflet/lib/index.js: Unknown option: /.../node_modules/react-leaflet/.babelrc.optional while parsing file: /.../node_modules/react-leaflet/lib/index.js

@SteveALee
Copy link

It seems to me that as this is an official hapi module there should be an official hapi policy in place for module dependencies.

My vote is make it work for the widest possible audience, out of the box and with least effort. At least until ES5 is way down the long tail.

@nlf
Copy link
Collaborator

nlf commented Dec 15, 2015

we have a discussion along those lines going here

@ericclemmons
Copy link

Had the same problem & spent more time researching it than it took to fix it with webpack:

module: {
  loaders: [
        {
          exclude: /node_modules/,
          loader: "babel-loader",
          test: /\.js$/,
        },
        {
          include: /node_modules\/qs/),
          loader: "babel-loader",
          test: /\.js$/,
        },
    ],
  }
}

Happy copy-pasting!

@arb
Copy link
Contributor

arb commented Jan 10, 2016

I think you could also replace include: /node_modules\/qs/) with include: require.resolve('qs') as well which would be a little clearer what you're trying to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants