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

Lazily require abort-controller #478

Merged
merged 2 commits into from Jul 11, 2022
Merged

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented Jul 9, 2022

It throws when it has been bundled, then loaded in node.

How I found this:

  • Updating dependencies in cbor
  • In the cbor-web sub-package, I use webpack to pre-bundle cbor for easy use in the browser
  • I then spot-check that webpack'd version in node to ensure that it works as expected
  • This error: TypeError: Cannot destructure property 'AbortController'
  • Tracked it back to code in abort-controller, which should only get used in the browser, but because I'm running a bundle, I hit it in node
  • Backtracked into readable-stream. There's no reason to require abort-controller unless globalThis.AbortController doesn't exist
  • Moving the require after the globalThis.AbortController check fixes my issue

This is probably an edge case that I hope nobody else runs into, but the fix for my problem shouldn't hurt anyone else.

@hildjj
Copy link
Contributor Author

hildjj commented Jul 9, 2022

It looks like your Browsers / Browsers (macos-latest, firefox, webpack) test may have a timing issue. I saw a failure of that test in my fork that went away when I re-ran that test.

@mcollina
Copy link
Member

mcollina commented Jul 9, 2022

Could you do the changes in the build script and re-run them?

Take a look at https://github.com/nodejs/readable-stream/blob/main/build/replacements.mjs.

@hildjj
Copy link
Contributor Author

hildjj commented Jul 9, 2022

Things I didn't fix (yet) because they aren't germane to the issue at hand:

@mcollina mcollina merged commit c04c4db into nodejs:main Jul 11, 2022
@mcollina
Copy link
Member

Good work!

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.

None yet

2 participants