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

refactor: replace promises with async/await #496

Merged
merged 1 commit into from
Dec 3, 2020
Merged

Conversation

TimBeyer
Copy link
Contributor

@TimBeyer TimBeyer commented Dec 2, 2020

BREAKING CHANGE: Some functions that used to throw synchronously for things like bad parameters now reject the promise.
In many cases users may not have to do anything assuming the calls already happened within a promise chain, but in rare cases this may need some refactoring of error handling cases.

@codecov
Copy link

codecov bot commented Dec 2, 2020

Codecov Report

Merging #496 (0399242) into master (e52ca41) will increase coverage by 2.85%.
The diff coverage is 96.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #496      +/-   ##
==========================================
+ Coverage   62.50%   65.35%   +2.85%     
==========================================
  Files          12       12              
  Lines        1696     1882     +186     
  Branches      279      341      +62     
==========================================
+ Hits         1060     1230     +170     
- Misses        636      652      +16     
Impacted Files Coverage Δ
lib/paged-sync.js 77.61% <95.65%> (+1.42%) ⬆️
lib/create-contentful-api.js 84.71% <96.15%> (+3.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e52ca41...0399242. Read the comment docs.

@TimBeyer TimBeyer force-pushed the refactor/async-await branch from 0445779 to 2d0c884 Compare December 2, 2020 14:29
Copy link
Member

@marcolink marcolink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PR title should start with BREAKING CHANGE: to trigger a major release
https://github.com/semantic-release/semantic-release#commit-message-format

Would it make sense to add a migration section to the readme?

@TimBeyer
Copy link
Contributor Author

TimBeyer commented Dec 2, 2020

According to https://www.conventionalcommits.org/en/v1.0.0/ the breaking change section goes into the footer or you add the exclamation mark after the type.

@TimBeyer
Copy link
Contributor Author

TimBeyer commented Dec 2, 2020

Not sure what to write in a migration section. In theory this does not need changes in the vast majority of setups unless maybe the whole babel and regenerator stuff needs to be added somewhere. I'm not very familiar with those.

But overall the breaking changes are only in the validation of parameters for the sync calls from what I can tell, so you would probably only encounter those during development and not once things are fully set up.

BREAKING CHANGE: Some functions that used to throw synchronously for things like bad parameters now reject the promise.
In many cases users may not have to do anything assuming the calls already happened within a promise chain, but in rare cases this may need some refactoring of error handling cases.
@TimBeyer TimBeyer force-pushed the refactor/async-await branch from 2d0c884 to 0399242 Compare December 3, 2020 08:59
@TimBeyer TimBeyer merged commit eb41a64 into master Dec 3, 2020
@TimBeyer TimBeyer deleted the refactor/async-await branch December 3, 2020 09:04
@ghost
Copy link

ghost commented Dec 3, 2020

🎉 This PR is included in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ghost ghost added the released label Dec 3, 2020
@mduleone
Copy link

mduleone commented Dec 3, 2020

Hello contentful team! Question about this change: Previously, we were able to run contentful in Node (v12.3.1) without issue, and now the same script is throwing an error. I understand that this includes breaking changes, but what would you recommend as the course of action to resolve something like this?

(node:72864) UnhandledPromiseRejectionWarning: ReferenceError: regeneratorRuntime is not defined
    at Object._getEntries (/Users/mattduleone/sites/next-web/node_modules/contentful/dist/contentful.node.js:6213:51)
    at Object.getEntries (/Users/mattduleone/sites/next-web/node_modules/contentful/dist/contentful.node.js:6191:24)
    at fetchContentfulData (/Users/mattduleone/sites/next-web/bin/contentful-refresh.js:59:33)
    at populateData (/Users/mattduleone/sites/next-web/bin/contentful-refresh.js:90:29)
    at Object.<anonymous> (/Users/mattduleone/sites/next-web/bin/contentful-refresh.js:111:1)
    at Module._compile (internal/modules/cjs/loader.js:774:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:785:10)
    at Module.load (internal/modules/cjs/loader.js:641:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:837:10)

@TimBeyer
Copy link
Contributor Author

TimBeyer commented Dec 3, 2020

Oh that was definitely an unintended side effect of this change.
For now to unblock you, you could use https://www.npmjs.com/package/regenerator-runtime directly in your codebase.
For a proper fix I think we will bump the minimum node version required to run this because it seems a bit much to make everyone on a modern version of node use regenerator when all of this is natively supported.

@TimBeyer
Copy link
Contributor Author

TimBeyer commented Dec 3, 2020

@mduleone we have a fix prepared in #498 that bumps the minimum version of node and thus gets rid of the dependency for regenerator.

@mduleone
Copy link

mduleone commented Dec 3, 2020

@TimBeyer thank you! Testing this out now!

@mduleone
Copy link

mduleone commented Dec 3, 2020

@TimBeyer Everything is working as expected. Thank you again!

@jesperp
Copy link

jesperp commented Dec 20, 2020

Hi, also getting the same error as @mduleone (ReferenceError: regeneratorRuntime is not defined), but inside the browser! (using Svelte, Typescript, Snowpack)

I'm just passed fixing this error:
contentful/contentful-sdk-core#120

And now trying to get a simple client.getEntries call working but getting the regeneratorRuntime error. Installing the regenerator-runtime package and importing it fixes this but can this be solved by contentful? I'm a bit javascript fatigued after many strange errors leading way down the rabbit hole of SO posts and github issues. Maybe the error is with svelte preprocessing / snowpack, I don't know! Hope to get some light on this issue anyhow!

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

Successfully merging this pull request may close these issues.

5 participants