bug: use of top level awaits and node modules causes bunding errors

EisenbergEffect commented 3 months ago

Prerequisites

[X] I have written a descriptive issue title
[X] I have searched existing issues to ensure it has not already been reported

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 - Remove node: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

Setup a basic ES6 browser build with esbuild.
Import the polyfill in your index.js
Attempt to build

Expected Behavior

I would expect to be able to bundle without any errors.

fabiancook commented 3 months ago

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.

EisenbergEffect commented 3 months ago

I'll break this up into two PRs. The first for the UUID fix and the second for the JSON fix.

EisenbergEffect commented 3 months ago

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.

fabiancook commented 3 months ago

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.

EisenbergEffect commented 3 months ago

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.

fabiancook commented 3 months ago

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.

EisenbergEffect commented 3 months ago

Thanks for merging the UUID PR. I'll put together a second PR shortly for the other issue.

EisenbergEffect commented 2 months ago

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.

virtualstate / navigation