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

Fix & re-enable rs-wnfs integration tests #495

Merged
merged 2 commits into from
Feb 15, 2023
Merged

Conversation

matheus23
Copy link
Contributor

Fixes #429

I added the "no changelog" label since this is only a fix for a feature behind an experimental feature flag. I don't think this needs a changelog entry. Right?

@matheus23 matheus23 added the no changelog Attached to PRs which shouldn't require changelog changes label Feb 14, 2023
@matheus23
Copy link
Contributor Author

I wrote more about the issue itself here:

rustwasm/wasm-bindgen#3307

@matheus23
Copy link
Contributor Author

So apparently, the original code already had that initialized variable which prevents the bug:
https://github.com/fission-codes/webnative/blob/2d0a93e4f435a51a7618ce47544fb17b391352ad/src/fs/v3/PublicRootWasm.ts#L19-L22

But then it got reverted in https://github.com/fission-codes/webnative/pull/422/files?diff=unified&w=0#diff-4952ebe25ed816f4c8f9f5d03618522f41ab5e00941e4a69f46990b1f9755094L19-R28

This is just reverting the revert.

I realize the refactor's goal when it removed the initialized variable tried to get rid of global mutable state in general. Unfortunately, in this case we can't get rid of it, as we need to be careful about correctly maintaining wasm-bindgen-generated code (which contains global mutable state), and which is somewhat out of our control. The best we can do is file an issue (see the previous message).

So, I added more comments signalling that this code must not be deleted.

Copy link
Contributor

@icidasset icidasset left a comment

Choose a reason for hiding this comment

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

Thanks! 🙏

@matheus23 matheus23 merged commit 3ae07b8 into main Feb 15, 2023
@matheus23 matheus23 deleted the matheus23/fix-wasm branch February 15, 2023 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Attached to PRs which shouldn't require changelog changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-enable WASM tests
2 participants