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 object-assign and bluebird dependencies #69

Closed
davej opened this issue Feb 1, 2016 · 25 comments
Closed

Remove object-assign and bluebird dependencies #69

davej opened this issue Feb 1, 2016 · 25 comments

Comments

@davej
Copy link
Contributor

davej commented Feb 1, 2016

Since pnpm is only compatible with node >= 4 anyway, why not use native promises and Object.assign()?

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

ahh... i think i found a bug on Promise.all that bluebird fixed, hence the need for bluebird. i'll have to investigate a little more on that.

I'd still like to keep bluebird as a devdependency though. its stack traces are super useful.

@davej davej closed this as completed Feb 1, 2016
@rstacruz rstacruz reopened this Feb 1, 2016
@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

the object-assign makes sense!

actually, i've been thinking on whether it should support legacy node.js versions too... not sure on that. being able to use arrow functions really cleans up the source considerably.

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

My $0.02 is to keep the source clean for the moment and use the ES6 features that are available. Could always do a babelified mirror for older versions of node if there's a demand for it.

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

actually i can probably do something like this:

{
  "prepublish": /* convert lib/ to babel-ified versions */
  "postpublish": /* reset lib/ */
}

in which case it makes sense to keep bluebird/object-assign.

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

Yup, I prefer the idea of doing this for a node 0.x fork though, and keeping the main lib unbabelified.

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

Babel can polyfill Promises and Object.assign too by the way. Although perhaps it's best to keep bluebird for the reasons you mentioned.

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

Babel can polyfill Promises and Object.assign too by the way

using require('babel-polyfill'), afaik. I'd rather keep that out :)

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

using require('babel-polyfill'), afaik. I'd rather keep that out :)

With Babel6, you selectively choose the polyfills so it's not much different than the object-assign polyfill currently being used. https://babeljs.io/docs/plugins/transform-object-assign/

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

okay, that's quite handy.

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

Just going to lodge a 👎 on babelifying the main lib. :-)

I'd prefer to keep the node 0.x version as a fork. Babel adds a layer of abstraction (albeit lightweight) to debugging and stacktraces.

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

I suppose sourcemaps negate that a little bit but I still prefer the idea of keeping the main lib without it.

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

I'd prefer to keep the node 0.x version as a fork.

wouldn't a hard fork present a problem?

how about publishing a babelified version as "pnpm-legacy"?

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

how about publishing a babelified version as "pnpm-legacy"?

Yup, that's what I meant. Fork was probably a confusing word to use. :-)

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

oh there's also --retain-lines, how aboout that one?

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

also, there's an idea of something like

install = isNodeVersion4orAbove
  ? require('lib/cmd/install')
  : require('lib.es5/cmd/install')

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

oh there's also --retain-lines, how aboout that one?

I'd prefer to just use sourcemaps over --retain-lines, if it was between the two.

install = isNodeVersion4orAbove
  ? require('lib/cmd/install')
  : require('lib.es5/cmd/install')

Are you bundling two versions of the lib in that case (es5 and es6)? My preference would still be to have a separate pnpm-legacy module.

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

yeah. my only concern with publishing a pnpm-legacy module is that it isn't putting the users ahead first: it's essentially pushing the burden on the user.

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

I'd prefer to just use sourcemaps over --retain-lines, if it was between the two.

Does (modern) node.js support source maps?

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

Does (modern) node.js support source maps?

Yup

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

#73 now adds inline source maps as part of the babel experiment. No idea if it works, haha.

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

yeah. my only concern with publishing a pnpm-legacy module is that it isn't putting the users ahead first: it's essentially pushing the burden on the user.

Yeah, I look at it another way. You would be treating node >= 4 as a first class citizen, while still providing a path for Node 0.x users. The marketshare for 0.x will only continue to diminish over time. You can always decide to move the legacy version into core in the future if there's a demand... but it's much more difficult to move the other way because there will probably be people relying on 0.x support.

Just my 2 cents, obviously it's your module and your call though :-)

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

Also, you can always console.log a descriptive error message telling users to install pnpm-legacy when run on node 0.x. It's not much of a roadblock really.

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

on the other hand it'd cause fragmentation. keep in mind the CLI isn't the only way to consume pnpm—say, if ember-cli were to add pnpm support, they'd likely opt to use pnpm-legacy instead.

@davej
Copy link
Contributor Author

davej commented Feb 1, 2016

Yeah, but the nice thing is that pnpm-legacy would be phased out over time. There may be a demand for it now but perhaps not so much in a couple of years time. And when demand goes away then you're not left in a position where you still need to transpile to support 1% of the user-base who are still using node 0.x.

Anyway, I see pros and cons both ways and my opinion is more of a personal preference really :-)

@rstacruz
Copy link
Member

rstacruz commented Feb 1, 2016

I couldnt get the object-assign transform to work, btw. bummer

@rstacruz rstacruz closed this as completed Feb 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants