search

wasm-tool / parcel-plugin-wasm.rs

wasm-bindgen support for Parcel bundler
MIT License
66 stars 16 forks source link

Update to Parcel 1.11.0 #14

Closed mvlabat closed 5 years ago

mvlabat commented 5 years ago

Parcel switched from toml to @iarna/toml. So this update should fix #13

ishitatsuyuki commented 5 years ago

Please add a direct dependency.

Or, in other words: do not require anything that is not in dependency or peerDependency. parcel-bundler should actually be a peer (in addition to dev) dependency, but I will leave it up to you to change it in this PR or not.

mvlabat commented 5 years ago

Thanks for the comment! I actually agree with you on this. Didn't add it at first as it was the same for toml, but I assume we could avoid this problem at all if it was added as a dependency from the beginning. Fixed

mvlabat commented 5 years ago

@ishitatsuyuki Just had to rollback making parcel-bundler a peer dependency. It seems like it breaks my local project. Can we require a peer dependency in the plugin code? I've never used peer dependencies, so I'm not really sure how they work.

ishitatsuyuki commented 5 years ago

Pardon me, rollback what? What's broken?

The dependency thing I mentioned is a coding practice, and even if you don't follow it the code will accidentally work due to NPM's flat install architecture. It can break later like this case, of course.

catsigma commented 5 years ago

@mvlabat Thanks for the PR.

Should be better if we move the require('@iarna/toml') under the this.isNormalTOML() equals to true condition.

Then we can have our package.json without @iarna/toml needed.

ishitatsuyuki commented 5 years ago

@catsigma No, that is not good. That's against the Node.js module resolution algorithm.

See https://pnpm.js.org/docs/en/faq.html#pnpm-does-not-work-with-your-project-here for some explanation.

mvlabat commented 5 years ago

@ishitatsuyuki, sorry that was my own mistake, disregard my last comment. Could someone else test it with https://github.com/rustwasm/rust-parcel-template as well? Updated plugin now works for me, but I would like somebody else to confirm it before merging.