Skip to content

Conversation

fzhinkin
Copy link
Collaborator

Leftovers from #265

@fzhinkin fzhinkin requested a review from whyoleg March 19, 2024 09:46
Copy link
Contributor

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

LGTM
But, is there a reason why Wasm and JS implementations are different?
In my experiments current approach works for both Wasm and JS, and so the implementation looks similar (except for minor differences in jsinterop for wasm vs js), so it's easier to understand the code and so could mostly copy-pasted

@fzhinkin
Copy link
Collaborator Author

But, is there a reason why Wasm and JS implementations are different?

There was a reason, but now it seems like it should be possible to unify it.

@fzhinkin fzhinkin merged commit 20b6bd7 into develop Mar 19, 2024
@fzhinkin
Copy link
Collaborator Author

Filed #287 for unifying module loading

@fzhinkin fzhinkin deleted the avoid-webpack-warnings branch March 19, 2024 15:31
@lppedd
Copy link
Contributor

lppedd commented Apr 9, 2024

I've missed the discussion about this change. Why couldn't Webpack simply be configured to mute those warnings?

E.g., configure externals, or use the ignore plugin.

@fzhinkin
Copy link
Collaborator Author

@lppedd If I understand correctly, it could be done on the library user side only, which is kinda lame. Instead, the root cause of the warning was fixed (well, not fixed, but muted).

@lppedd
Copy link
Contributor

lppedd commented Apr 27, 2024

@fzhinkin Thanks. I get that not displaying a warning consumer side is nice, but isn't it better to make the consumer aware of what is going on instead of hiding it? I mean, once I start working with K/JS I know that Webpack runs for browser, so I can fiddle with its configuration in case. This could be highlighted in the readme.

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.

3 participants