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

Breaking change removing bluebird #266

Closed
AlexisNo opened this issue Feb 20, 2017 · 5 comments
Closed

Breaking change removing bluebird #266

AlexisNo opened this issue Feb 20, 2017 · 5 comments

Comments

@AlexisNo
Copy link

AlexisNo commented Feb 20, 2017

Hi, thanks for the great work on this lib.

The removal of bluebird introduce a breaking change. I think the version number of the lib should have changed accordingly.

Indeed, the spread method disappeared.

workbook.xlsx.readFile(file)
.then(() => {
  return Promise.all([
   asyncOpA(),
   asyncOpB()
  ])
})
.spread((resA, resB) => {
  //...
);

TypeError: [...].spread is not a function

I think the use of spread is very common for bluebird users. I my case, my build was broken by this minor version change.

Ideally, we should have the possibility to inject bluebird in ExcelJS to continue to use it.

@guyonroche
Copy link
Collaborator

My sincerest apologies, I'd forgotten that bluebird was part of the interface.
Let me have a think - there may be a solution to this without pulling bluebird back in - it really does come with a massive footprint that I'd like to avoid going forward.

In the mean time - which node version are you using? if >=6.x you should be able to change that line with:

  .then(([resA, resB]) => { /* ... */ }

@guyonroche
Copy link
Collaborator

@AlexisNo - ok, I have an alternative solution that should offer most of what bluebird did at a fraction of the footprint.
Can you (or anyone else) confirm which bluebird features you are using?

@AlexisNo
Copy link
Author

AlexisNo commented Feb 21, 2017

Personally, I like to use spread() and map().

I think it is possible to exclude bluebird and allow the user to inject a Promise lib. Mongoose has this functionality. I implemented it for a small module too. This way you do not need to pull bluebird back in and users are free to use their favorite Promise implementation.

Thanks for the fix. It is true that this ES6 syntax makes spread() irrelevant. I used another solution:

const Promise = require('bluebird');
return Promise.resolve(workbook.xlsx.readFile(file))
.then( /* ... */ )
.spread( /* ... */ )

It looks more like a hack, but I don't need to change every spread() or map() in the promise chain.

@guyonroche
Copy link
Collaborator

Once again - sorry for the break.
I have published 0.3.0 with the following two changes:

  • A richer internal Promise implementation (that supports some of the bluebird extras including spread and map)
  • Promise library dependency injection to allow you to restore bluebird if needed.

@AlexisNo
Copy link
Author

Awesome! Thank for the quick fix.
And again, thanks for the great job on this lib.

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