-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
bug: use of top level awaits and node modules causes bunding errors #32
Comments
Am happy with both changes if you're up for removing the external dependency code. For UUID if there is a global alternative available, it should be the preferred, e.g. in a browser. It would be ideal if we could provide the id generator for directly constructed navigation, but can be a later change. This would allow the node uuid generator to be still used optionally. If global polyfill and wanting a custom generator, applyPolyfill with a navigation instance would be used. I had tried to get these working in a way that it worked for all, but seems like it just keeps biting as an issue. |
I'll break this up into two PRs. The first for the UUID fix and the second for the JSON fix. |
I'm having a heck of a time getting this to install, build, and test correctly. I'll try to put together a PR and then for the sake of simplicity perhaps we can validate it with the CI. It's going to take me longer to figure out the local setup than to create the fixes. |
Yeah that's not expected or a great experience at all. What OS are you on? I'll get a a code space set up for it and make sure it installs. |
I'm on Windows. So, I hit some issues there. There was also an issue with the WPT dependency. After hacking around those, I hit some more snags. I can't recall. |
I'll see about cross compatibility for windows on the build. Sorry about this, I know how frustrating not being able to just ran the project is. |
Thanks for merging the UUID PR. I'll put together a second PR shortly for the other issue. |
Just an FYI that I still intend to put together the 2nd PR mentioned above. Just had a few other things I needed to address. Coming back to this soon. |
We'd also like to use this polyfill as an option in our webpack based app. I'd be happy to test on webpack also to get the follow-up PR: #35 -submited by @EisenbergEffect approved. |
I was able to get the polyfill working with webpack by adding target: ['web', 'es2022'], To my webpack.config.js |
Ok, finally got an initial PR in to fix the second issue. I need some help from maintainers to get it over the line. I had issues on Windows running tests and wasn't quite clear how best to update the custom build scripts now that I've removed some of the code. |
Prerequisites
Version (i.e. v2.x.x)
All
Node.js version (i.e. 18.x, or N/A)
No response
Operating system
None
Operating system version (i.e. 20.04, 11.3, 10, or N/A)
No response
Description
There are a number of issues when trying to use this polyfill with various bundlers. In particular, top level awaits related to dynamically importing other modules and Node dependencies create issues. I believe these can be easily resolved by simplifying the two places that cause the problem:
util/uuid-or-random.ts
- Removenode:crypto
. I'm not sure you need a cryptographically random number in your scenarios. Just use the fake v4 implementation or something similar.util/structured.json
- This appears to only be using basic parsing and stringification, not structured cloning. Remove this and just use built-in JSON APIs directly.These changes fix the errors and will reduce code size and complexity. I'm happy to make a PR with the proposed changes if you are interested. As it stands, this poyfill does not work with esbuild or any build systems based on esbuild.
Steps to Reproduce
Expected Behavior
I would expect to be able to bundle without any errors.
The text was updated successfully, but these errors were encountered: