-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
bump webassemblyjs #7732
bump webassemblyjs #7732
Conversation
For maintainers only:
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
Please do not merge this yet. |
Ping me when this is ready to merge |
@@ -9,7 +9,6 @@ const Template = require("../Template"); | |||
const WebAssemblyUtils = require("./WebAssemblyUtils"); | |||
const { RawSource } = require("webpack-sources"); | |||
|
|||
const { shrinkPaddedLEB128 } = require("@webassemblyjs/wasm-opt"); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Alright @sokra, lgtm |
yarn.lock
Outdated
"@webassemblyjs/wasm-gen" "1.5.13" | ||
"@webassemblyjs/wasm-parser" "1.5.13" | ||
debug "^3.1.0" | ||
"@babel/cli" "^7.0.0-beta.54" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
babel should be a devDependency
yarn.lock
Outdated
version "1.7.4" | ||
resolved "https://registry.yarnpkg.com/@webassemblyjs/ieee754/-/ieee754-1.7.4.tgz#b58c92c364a954a027d7415adc1bf10dd74d7ac5" | ||
dependencies: | ||
"@xtuc/buffer" "^5.2.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node.js has a buildin Buffer.
This seem to be for browser build only. Bundlers usually bring their own polyfills for Buffer. Polyfilling is not the responsibility of a library. If it's only needed to local building of a browser bundle, make it a dev dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right. Let me remove it.
yarn.lock
Outdated
version "7.0.0-beta.54" | ||
resolved "https://registry.yarnpkg.com/@babel/code-frame/-/code-frame-7.0.0-beta.54.tgz#0024f96fdf7028a21d68e273afd4e953214a1ead" | ||
dependencies: | ||
"@babel/highlight" "7.0.0-beta.54" | ||
|
||
"@babel/core@^7.0.0-beta.54": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not be included
"@webassemblyjs/ast" "1.5.13" | ||
"@webassemblyjs/wast-parser" "1.5.13" | ||
long "^3.2.0" | ||
"@xtuc/long@4.2.1": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird that you are forking packages just for ESM. This seems unnecessary to me. Why not using the original package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the issue is that some bundler (Rollup) won't bundle any cjs require, so I had to ESM'ify some dependecies.
For now I preferred forking them to avoid loosing my time merging it upstream, but that's definitely the plan. Is that a blocker for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rollup has a commonjs plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sokra I submited PRs the upstream repositories. Would you mind removing them in a following PR? so we can fix the current issues in Webpack. If that's ok to you i'll update the PR to be ready to be merged.
Please merge this soon after @xtuc considers it final; the current implementation actively corrupts binaries in certain circumstances. (rustwasm/wasm-bindgen#753) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xtuc/long
should eventually merged into upstream, but we can use it for now.
Please let me update webassemblyjs before merging this. |
I updated it to 1.7.5 |
Do you want to cut a new release? |
Ah cool thanks, yeah I just released 1.7.6 (mostly changes to the interpreter, which is not used here). |
Thanks |
What kind of change does this PR introduce?
Did you add tests for your changes?
Does this PR introduce a breaking change?
What needs to be documented once your changes are merged?
ref xtuc/webassemblyjs#407