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

feat: polyfill AbortController, so that Ceramic node works on Node.js v14 #2090

Merged
merged 6 commits into from
Mar 24, 2022

Conversation

ukstv
Copy link
Contributor

@ukstv ukstv commented Mar 21, 2022

Polyfill AbortController, absent on Node.js v14.

This seems to work. Tests are green on my machine. On CircleCI I see quite cryptic errors. This seems to be related to these issues: ipfs/js-ipfs#3998, websockets/ws#1818. That is the reason to drop testing with Node14 on CircleCI.

@PaulLeCam Maybe you could give a shot with using it on AWS, after the release?

Copy link
Contributor

@stbrody stbrody left a comment

Choose a reason for hiding this comment

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

core changes seem reasonable. My main question is what happens when a developer starts using AbortController/AbortSignal in a new file and forgets to call polyfillAbortController() at the top of the file? If this isn't caught in code review, is there any way we will notice before users complain or deploying to Self.ID fails (which may happen long after the corresponding Ceramic change is released)?

@ukstv
Copy link
Contributor Author

ukstv commented Mar 23, 2022

What we do here with AbortController, is an attempt to not re-do the whole deployment scheme of self.id when it migrates to Ceramic v2. We are not going to support Node.js v14 generally. We expect that in few months, we could forget about Node.js v14 completely due to imminent support of Node16 by AWS (ETA 2months from now). It might be the case, that the current hack will not work. In this case, self.id has to stay on Ceramic v1, and we forget about even this attempt of Node14 support.

Future changes is a perfectly valid concern. If I am not mistaken, a generally accepted practice is to polyfill before doing anything in your app (not a library!). I guess, the way to go is for @PaulLeCam to call polyfillAbortController function in the app entrypoint (_app.ts for Next.js, if I am not mistaken). Then AbortController would be available to the whole process.

@PaulLeCam
Copy link
Contributor

Yes shouldn't be a problem to add the AbortController polyfill to the app itself 👍

Copy link
Contributor

@stbrody stbrody left a comment

Choose a reason for hiding this comment

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

Okay so sounds like the plan is, if we accidentally slip in new uses of AbortController without polyfilling them in js-ceramic, and then some user complains that they can no longer run Ceramic with Node 14, we'll just tell them to manually pollyfill AbortController themselves, and that should get them working again. Seems reasonable.

@ukstv ukstv merged commit fff3e8a into develop Mar 24, 2022
@ukstv ukstv deleted the su/abort-controller-2 branch March 24, 2022 12:33
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

Successfully merging this pull request may close these issues.

3 participants