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

Don't use "./node_modules/" in the import path #151

Merged
merged 1 commit into from
Apr 19, 2018

Conversation

mstange
Copy link
Contributor

@mstange mstange commented Apr 19, 2018

When using webpack, modules from dependencies are usually not included through their actual paths on the file system. Instead, the include path starts with the name of the module and it's webpack's job to find the correct directory.

On the other hand, having the full path here is nice because it tells the reader where to go looking for the actual files that wasm-pack produced. So I'm not sure if this is a good change.

When using webpack, modules from dependencies are usually not included through their actual paths on the file system. Instead, the include path starts with the name of the module and it's webpack's job to find the correct directory.
@mgattozzi
Copy link
Contributor

Hey @mstange just want to make sure, does this run correctly with wasm-pack right now?

My understanding is we'll need to land a fix to close https://github.com/ashleygwilliams/wasm-pack/issues/94 first and release a version 0.2.0

@mstange
Copy link
Contributor Author

mstange commented Apr 19, 2018

I've tested this locally and it works correctly. This change is only changing it to @MYSCOPE/wasm-add/wasm_add.js. Once we have ashleygwilliams/wasm-pack#94, we'll be able to shorten it even more, to @MYSCOPE/wasm-add.

@mgattozzi
Copy link
Contributor

Ok excellent just wanted to confirm. Let's get this merged. Thanks for the fix @mstange

@mgattozzi mgattozzi merged commit 2e679c5 into rustwasm:master Apr 19, 2018
@ashleygwilliams
Copy link
Member

🎉 yay thank u!

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.

4 participants