-
Notifications
You must be signed in to change notification settings - Fork 15
fix(cli): #1723 gate import.meta.url bundling on js content-type so unsupported resource skip acorn parsing #1724
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -320,7 +320,12 @@ function greenwoodImportMetaUrl(compilation) { | |
| } | ||
| } | ||
|
|
||
| if (!canTransform) { | ||
| // only parse resources that were actually served as JavaScript, otherwise binary resources | ||
| // (e.g. .wasm / .png / .woff2) get fed to acorn and crash the build with a cryptic SyntaxError | ||
| // https://github.com/ProjectEvergreen/greenwood/issues/1723 | ||
| const contentType = response.headers.get("content-type") || ""; | ||
|
|
||
| if (!canTransform || !contentType.includes("text/javascript")) { | ||
| return null; | ||
| } | ||
|
|
||
|
|
@@ -363,7 +368,9 @@ function greenwoodImportMetaUrl(compilation) { | |
| if (plugin.shouldServe && (await plugin.shouldServe(url, request))) { | ||
| const response = await plugin.serve(url, request); | ||
|
|
||
| if (response?.headers?.get("content-type") || "".indexOf("text/javascript") >= 0) { | ||
| // parenthesize so we test the content-type string contains "text/javascript" | ||
| // https://github.com/ProjectEvergreen/greenwood/issues/1723 | ||
| if ((response?.headers?.get("content-type") || "").indexOf("text/javascript") >= 0) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh wow, good catch! |
||
| bundleExtensions = [...bundleExtensions, ...plugin.extensions]; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /* | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I wonder if it might make more sense to combine both of these test cases? Seems like the are pretty related and we could just all the same related behavior under one suite. Just thinking out loud. |
||
| * Use Case | ||
| * Run Greenwood build command with an unsupported direct `import ... from "./add.wasm"`. | ||
| * | ||
| * User Result | ||
| * The build should fail, but NOT with the misleading acorn `SyntaxError` thrown from inside the | ||
| * `greenwood-import-meta-url` Rollup plugin (which pointed users at internal plugin code when the | ||
| * wasm binary was fed to the JavaScript parser). A clear, Rollup-native parse error is acceptable. | ||
| * | ||
| * User Command | ||
| * greenwood build | ||
| * | ||
| * User Config | ||
| * None (Greenwood Default) | ||
| * | ||
| * User Workspace | ||
| * src/ | ||
| * pages/ | ||
| * index.html | ||
| * scripts/ | ||
| * add.wasm | ||
| * main.js | ||
| */ | ||
| import { expect } from "chai"; | ||
| import path from "node:path"; | ||
| import { getOutputTeardownFiles } from "../../../../../test/utils.js"; | ||
| import { Runner } from "gallinago"; | ||
| import { fileURLToPath } from "node:url"; | ||
|
|
||
| describe("Build Greenwood With: ", function () { | ||
| const cliPath = path.join(process.cwd(), "packages/cli/src/bin.js"); | ||
| const outputPath = fileURLToPath(new URL(".", import.meta.url)); | ||
| let runner; | ||
| let buildError; | ||
|
|
||
| before(function () { | ||
| this.context = { | ||
| publicDir: path.join(outputPath, "public"), | ||
| }; | ||
| runner = new Runner(); | ||
| }); | ||
|
|
||
| describe("An unsupported direct import of a .wasm module", function () { | ||
| before(async function () { | ||
| await runner.setup(outputPath); | ||
|
|
||
| try { | ||
| await runner.runCommand(cliPath, "build"); | ||
| } catch (error) { | ||
| buildError = `${error}`; | ||
| } | ||
| }); | ||
|
|
||
| it("should fail the build", function () { | ||
| expect(buildError).to.not.equal(undefined); | ||
| }); | ||
|
|
||
| // https://github.com/ProjectEvergreen/greenwood/issues/1723 | ||
| it("should not crash with the acorn SyntaxError from the greenwood-import-meta-url plugin", function () { | ||
| expect(buildError).to.not.contain("greenwood-import-meta-url"); | ||
| }); | ||
| }); | ||
|
|
||
| after(async function () { | ||
| await runner.teardown(getOutputTeardownFiles(outputPath)); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| <!doctype html> | ||
| <html> | ||
| <head> | ||
| <title>wasm direct import</title> | ||
| <script type="module" src="../scripts/main.js"></script> | ||
| </head> | ||
| <body> | ||
| <h1>wasm</h1> | ||
| <span id="out"></span> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import * as wasm from "./add.wasm"; | ||
|
|
||
| document.getElementById("out").textContent = `add(2,3)=${wasm.add(2, 3)}`; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| /* | ||
| * Use Case | ||
| * Run Greenwood build command referencing a .wasm asset via the supported | ||
| * `new URL("./add.wasm", import.meta.url)` pattern. | ||
| * | ||
| * User Result | ||
| * Should generate a Greenwood build that emits the .wasm as a content-hashed, | ||
| * byte-identical asset, without crashing the JavaScript parser on the binary. | ||
| * | ||
| * User Command | ||
| * greenwood build | ||
| * | ||
| * User Config | ||
| * None (Greenwood Default) | ||
| * | ||
| * User Workspace | ||
| * src/ | ||
| * pages/ | ||
| * index.html | ||
| * scripts/ | ||
| * add.wasm | ||
| * main.js | ||
| */ | ||
| import { expect } from "chai"; | ||
| import fs from "node:fs/promises"; | ||
| import path from "node:path"; | ||
| import { runSmokeTest } from "../../../../../test/smoke-test.js"; | ||
| import { getOutputTeardownFiles } from "../../../../../test/utils.js"; | ||
| import { Runner } from "gallinago"; | ||
| import { fileURLToPath } from "node:url"; | ||
|
|
||
| // https://github.com/ProjectEvergreen/greenwood/issues/1723 | ||
| describe("Build Greenwood With: ", function () { | ||
| const LABEL = "Referencing a .wasm asset with new URL and import.meta.url"; | ||
| const cliPath = path.join(process.cwd(), "packages/cli/src/bin.js"); | ||
| const outputPath = fileURLToPath(new URL(".", import.meta.url)); | ||
| let runner; | ||
|
|
||
| before(function () { | ||
| this.context = { | ||
| publicDir: path.join(outputPath, "public"), | ||
| }; | ||
| runner = new Runner(); | ||
| }); | ||
|
|
||
| describe(LABEL, function () { | ||
| before(async function () { | ||
| await runner.setup(outputPath); | ||
| await runner.runCommand(cliPath, "build"); | ||
| }); | ||
|
|
||
| runSmokeTest(["public"], LABEL); | ||
|
|
||
| describe("WASM asset output", function () { | ||
| let wasmFiles; | ||
|
|
||
| before(async function () { | ||
| wasmFiles = await Array.fromAsync( | ||
| fs.glob("add.*.wasm", { cwd: new URL("./public/", import.meta.url) }), | ||
| ); | ||
| }); | ||
|
|
||
| it("should emit exactly one content-hashed .wasm asset", function () { | ||
| expect(wasmFiles.length).to.equal(1); | ||
| }); | ||
|
|
||
| it("should emit the .wasm asset byte-identical to the source", async function () { | ||
| const emitted = await fs.readFile(new URL(`./public/${wasmFiles[0]}`, import.meta.url)); | ||
| const source = await fs.readFile(new URL("./src/scripts/add.wasm", import.meta.url)); | ||
|
|
||
| expect(emitted.equals(source)).to.equal(true); | ||
| }); | ||
|
|
||
| it("should reference the content-hashed .wasm filename from the bundled script", async function () { | ||
| const scripts = await Array.fromAsync( | ||
| fs.glob("main.*.js", { cwd: new URL("./public/", import.meta.url) }), | ||
| ); | ||
| const contents = await fs.readFile( | ||
| new URL(`./public/${scripts[0]}`, import.meta.url), | ||
| "utf-8", | ||
| ); | ||
|
|
||
| expect(contents).to.contain(wasmFiles[0]); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| after(async function () { | ||
| await runner.teardown(getOutputTeardownFiles(outputPath)); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| <!doctype html> | ||
| <html> | ||
| <head> | ||
| <title>wasm import.meta.url</title> | ||
| <script type="module" src="../scripts/main.js"></script> | ||
| </head> | ||
| <body> | ||
| <h1>wasm</h1> | ||
| <span id="out"></span> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| const wasmUrl = new URL("./add.wasm", import.meta.url); | ||
| const { instance } = await WebAssembly.instantiateStreaming(fetch(wasmUrl)); | ||
|
|
||
| document.getElementById("out").textContent = `add(2,3)=${instance.exports.add(2, 3)}`; |
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.
I see yeah, this makes sense to me.