Remove the bluebird dependency from everywhere 🔥 #14882
Comments
|
I can work with this, I had a similar task at my previous workplace 😁 |
Issue TryGhost#14882 - Bluebird promise was being used for getting cached image sizes using .props method. It has been removed to be replaced by native promise using Promise.all
|
I've updated the OP here to be clearer that we need to remove catch predicates first, and why as there have been a couple of PRs that missed that detail. |
Apologies for the delay, I've been away for a bit. Resuming working on this task today. |
refs: #14882 * refactored security.password to use native bcrypt promises * refactored security.string to use more modern es features
refs #14882 - catch predicates make removing Bluebird from other parts of the code risky.
refs: TryGhost#14882 - I found a common pattern where catch predicates were being used to catch non-existent models in destroy methods, and sometimes elsewhere in the API endpoints - The use of predicates is deprecated, and we're working to remove them from everywhere, so that we can remove bluebird - In order to still handle these errors correctly, we needed a small change to mw-error-handler so that it can detect EmptyResponse errors from bookshelf, as well as 404s Note: there is a small change as a result of this - the context on these errors now says "Resource not found" instead of "{ModelName} not found". - I think this is acceptable for now, as we will be reviewing these errors in more depth later. It's quite easy to make changes, we just have to decide what with proper design input
refs: TryGhost#14882 - The use of predicates is deprecated, and we're working to remove them from everywhere, so that we can remove bluebird - This should be the final piece of the puzzle in terms of predicates, from here we can start removing bluebird without concern that a predicate somewhere will explode - Note: some of this code is poorly tested, but the refactors are very straightforward and minimal
refs: #14882 - I found a common pattern where catch predicates were being used to catch non-existent models in destroy methods, and sometimes elsewhere in the API endpoints - The use of predicates is deprecated, and we're working to remove them from everywhere, so that we can remove bluebird - In order to still handle these errors correctly, we needed a small change to mw-error-handler so that it can detect EmptyResponse errors from bookshelf, as well as 404s Note: there is a small change as a result of this - the context on these errors now says "Resource not found" instead of "{ModelName} not found". - I think this is acceptable for now, as we will be reviewing these errors in more depth later. It's quite easy to make changes, we just have to decide what with proper design input
refs: #14882 - The use of predicates is deprecated, and we're working to remove them from everywhere, so that we can remove bluebird - This should be the final piece of the puzzle in terms of predicates, from here we can start removing bluebird without concern that a predicate somewhere will explode - Note: some of this code is poorly tested, but the refactors are very straightforward and minimal
|
Hey all 👋 this issue was blocked on needing to remove the catch predicates first, so I've just merged a PR that removes all of them. I've also updated the issue with the current state of things. I'll go through the couple of blocked open PRs that remove some bits of bluebird asap. In the meantime, it'd be great to get some momentum on this and get rid of bluebird finally! |
refs #14882 - we're nuking our use of Bluebird in favor of native Promises
refs #14882 - we're nuking our use of Bluebird in favor of native Promises, so this commit removes most of the usage we have in the core module
refs #14882 - removed Bluebird Promise.props from files in favor of native Promise.all
refs #14882 - this is no longer used
refs #14882 - this is no longer used so we can remove it as we move towards removing Bluebird from the codebase
refs #14882 - this is no longer used so we can remove it as we move towards removing Bluebird from the codebase
refs TryGhost#14882 with this the Bluebird is no longer required in ghost/core
|
Hello everyone, I'm a new contributor here! I've created pull request #17831 to remove Bluebird from the
I ran Thanks, |
refs TryGhost#14882 with this the Bluebird is no longer required in ghost/core
…previously returned by Promise.props refs TryGhost#14882
refs #14882 - this removes the last usage of Bluebird within Ghost and removes the dependency as it's no longer required within the codebase
refs TryGhost/Ghost#14882 Co-authored-by: Princi Vershwal <princi.vershwal@Princis-MacBook-Pro.local>
refs TryGhost/Ghost#14882 Co-authored-by: Princi Vershwal <princi.vershwal@Princis-MacBook-Pro.local>
refs TryGhost/Ghost#14882 - this dependency is no longer needed because we're removed all functions that were supplied from it
Also, update eslintrc to support arrow notation. Ref: TryGhost/Ghost#14882
Also, update eslintrc to support arrow notation. Ref: TryGhost/Ghost#14882
|
I've submitted a PR to the |
|
I've reviewed the status of this, and after the open PRs linked above are merged, it appears that perhaps than can be closed after lerna is used for final new package releases that contain the fix and and dependencies on those packages are updated one more time.
|
refs TryGhost/Ghost#14882 - this switches out the Bluebird specific code for a native variant so we can remove the dependency
|
kindly assign this to me , I can work on this |
|
@07tAnYa Per my comment above, there is really nothing left to do. What change are you planning to make? |
refs TryGhost/Ghost#14882 Co-authored-by: Daniel Lockyer <hi@daniellockyer.com>
Now that there are native promises in Node.js and with async/await, bluebird really is a blast from the past. However, it's embedded pretty hard across our dependencies and some of our code uses catch predicates, which means we have to be careful about the order in which we rip bluebird out, otherwise we could break our error handling in weird and unexpected ways.
Here's some notes on what we need to do, most importantly starting with finding everywhere we depend on catch predicates:
Native:
Bluebird
Refactor Approach
This refactor needs to be done in two phases:
💡 The best way to find catch predicates is with this regex:
/\.catch\([^)]+,/.Why did we have to do catch predicates first?
Catch predicates result in error handling that assume the Promise they are handling is a Bluebird promise. If that isn't the case, Ghost will error, and quite possibly crash.
Our code coverage isn't quite good enough to be certain that all the unhappy paths are tested and we'll find these, and it's too much cognitive load for anyone to figure out if removing Bluebird is safe whilst there are still catch predicates in the codebase.
How to work on this issue
The slow and steady approach to refactoring is the only thing that really works. Branches should be very short lived (<1 day if possible). Therefore, please do not attempt to do this entire refactor in a single PR. It is not possible for us to review!
Instead, please find a single file with many references, or a small subfolder of the codebase with a handful of usages, make the changes, and submit a PR. Then repeat.
Please follow our contribution guidelines as closely as you can, particularly being sure to reference this issue in your commit. Refactor commits should not use emojis as they are not user-facing changes.
Note: This refactor has almost no user facing impact and (hopefully) no tests should break or change.
A reference refactor commit can be found here: 10aad8d
Thank you 🙏 ❤️ 🙏
The text was updated successfully, but these errors were encountered: