search

ArweaveTeam / arweave-js

Browser and Nodejs client for general interaction with the arweave protocol and gateways
MIT License
588 stars 129 forks source link

fix: access to window #180

Closed Jack-Works closed 2 years ago

rosmcmahon commented 2 years ago

hi, this PR is failing tests

rosmcmahon commented 2 years ago

Specifically, it's not building. You can see the failing runs above.

What is this PR attempting to fix? Not clear what the issue is

Jack-Works commented 2 years ago

What is this PR attempting to fix? Not clear what the issue is

This library failed to run inside a Web Worker because there is no window variable in it.

Jack-Works commented 2 years ago

now i think it's ok

rosmcmahon commented 2 years ago

What is this PR attempting to fix? Not clear what the issue is

This library failed to run inside a Web Worker because there is no window variable in it.

IIRC there are other issues preventing this library from running inside web workers. Have you tested this PR locally?

Jack-Works commented 2 years ago

we have a patch to arweave-js https://github.com/DimensionDev/Maskbook/blob/develop/patches/arweave%401.11.4.patch to let it run in a Web Worker, but I didn't test all features. This PR just make it can be imported in a Web Worker without crash.

Jack-Works commented 2 years ago

I prefer a squash-merge 👀

rosmcmahon commented 2 years ago

I presume someone tested this with ArConnect or arweave.app @hlolli ?

hlolli commented 2 years ago

backstory please to this type of testing @rosmcmahon ?

hlolli commented 2 years ago

typescript compiles global window vars to window. That's what typescript does, I see your concern in that regard.

hlolli commented 2 years ago

ts: location.path js: window.location.path

rosmcmahon commented 2 years ago

yeah, i was thinking in more broad strokes actually:

rosmcmahon commented 2 years ago

we should probably do a quick test before publishing a new release, or is a scream test ok? 😅

Jack-Works commented 2 years ago

Does that break something? "self" is equal to "window" in a normal Web page and also available as the global object in a Web Worker, I thought it should be ok?

hlolli commented 2 years ago

as long as we aren't releasing major version I think a scream test is ok. There's no regression, api change or behvioral change in this as far as I can see. If there's one, it's going to have to be a big surprise.