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

ES6 style changes and node v4 #124

Closed
hueniverse opened this issue Nov 2, 2015 · 16 comments
Closed

ES6 style changes and node v4 #124

hueniverse opened this issue Nov 2, 2015 · 16 comments

Comments

@hueniverse
Copy link
Contributor

The hapi world is moving to only support node >= 4.0. This module is part of that world. I want to make sure the express community is aware of it as they are a significant user of this module. Also, this will affect use in the browser in pre-ES6 versions.

@dougwilson

@hueniverse hueniverse self-assigned this Nov 2, 2015
@hueniverse hueniverse added this to the 6.0.0 milestone Nov 2, 2015
@dougwilson
Copy link
Contributor

Hi @hueniverse , that sounds fine to me. We have no timeline to drop 0.10, unfortunately, and so we'll likely just fork this module for our ecosystems, if it becomes necessary. Likely, we'll sit on an old version unless there is some security issue :)

@hueniverse
Copy link
Contributor Author

If there are security issues we'll publish a 5.x patch since hapi 9.x will need it too.

hueniverse added a commit that referenced this issue Nov 3, 2015
@dougwilson
Copy link
Contributor

Even better :) Don't let us hold ya back ;)

emanchado added a commit to robohydra/robohydra that referenced this issue Nov 4, 2015
The "qs" dependency had a wrong version specified, and it was a
simple ">=". Unfortunately, the latest version of "qs", as of two
days ago, uses ES6 features so it won't be compatible with older
Nodes. As there's nothing in RoboHydra that requires a recent
version of Node (it seems to work even in Node 0.6.x), force the
"qs" dependency to be slightly older for now. According to
ljharb/qs#124 (comment) they
will provide security patches for the 0.5.x series, so we should be
fine.
cookpete added a commit to cookpete/fetch-reddit that referenced this issue Nov 6, 2015
qs now only supports node >= 4.0 and ES6 browsers, so this is a bit safer: ljharb/qs#124
@taion
Copy link

taion commented Nov 27, 2015

This is fairly unfriendly to people using this library v. e.g. browserify or webpack.

Have you considered still including a transpiled build in the npm package, then pointing the "browser" key in package.json to that?

@Marsup
Copy link
Contributor

Marsup commented Nov 27, 2015

This is fairly unfriendly to force maintainers to also maintain builds while you can make it yourself.

@taion
Copy link

taion commented Nov 27, 2015

Aren't you already maintaining a browser package at dist/qs.js? It looks like it hasn't been updated for 6.x yet and is still on 5.2.0, but you're going to have to pull in babelify to produce an updated browser bundle for 6.x that will actually run in browsers.

Either way, the reason this comes up is because in general when producing browser bundles, best practice is to exclude anything from node_modules for transpilation to avoid any dependencies on how exactly people are configuring Babel.

I like the approach you're taking with moving to use Node's ES6-supported features for Node code. It wouldn't be the end of the world if you dropped browser support in this library entirely, but right now you're actually shipping broken/outdated browser support via the distributed bundle, and would need to bring in Babel yourself to properly ship that bundle.

@nlf
Copy link
Collaborator

nlf commented Nov 27, 2015

You're right it is silly that we have an outdated browser build, but that's also why I have an open issue to remove that build. If you'd like to see browser support continued here, I'd be happy to hand the module over to you

@taion
Copy link

taion commented Nov 27, 2015

I just saw that issue, sorry.

I have a few more JS packages than I can get around to maintaining at the moment, unfortunately, but I'd be happy to contribute a PR that fixes the browser build if that's something you're interested in.

@nlf
Copy link
Collaborator

nlf commented Nov 27, 2015

I'm more inclined to accept a PR removing all the current browser support

@taion
Copy link

taion commented Nov 27, 2015

I see - thanks.

@evantahler
Copy link

Thanks for keeping v5.x around, and offering to publish security patches if they come up. Actionhero just switched to this library as well, and it's great! That said, we'll still be supporting node v0.10+ for another few months...

@mmahalwy
Copy link

mmahalwy commented Dec 4, 2015

I am having problems with this library breaking my tests cause I don't intend to bundle node_modules...

@ljharb
Copy link
Owner

ljharb commented Dec 21, 2015

It's very sad to have a module that is published in such a way that it requires transpilation to be used in a browser or older node environment. Why is it difficult to add a babel prepublish step?

@devinivy
Copy link

@ljharb feel free to check-out the discussion around this at outmoded/hapi-contrib#67

@ljharb
Copy link
Owner

ljharb commented Dec 21, 2015

Thanks, commented outmoded/hapi-contrib#67 (comment)

@hueniverse
Copy link
Contributor Author

use version 5. I'm locking this issue

Repository owner locked and limited conversation to collaborators Dec 22, 2015
@hueniverse hueniverse removed their assignment Dec 22, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants