core(build): add basic inline-fs plugin #13232
Conversation
connorjclark
requested review from
connorjclark
and removed request for
adamraine
There was a problem hiding this comment.
Great first pass! Most of the complexity coming up will be isolated to collapseToStringLiteral, which is nice.
Not many notes, just some nits.
re: "structured warnings", I'm not sure what you had in mind, but it'd be nice if some error in parsing (say, a failed assert) did not result in the entire thing failing.
To that end, I'd suggest:
- extracting the inner loop over
foundIndicesto a method and try/catching over it, collecting errors from parsing as they happen inlineFscould then returnPromise<{code: string|null, warnings: string[]}>(orwarnings?: string[]idk)
In other words, if we are going to return a list of warnings, we shouldn't also require the user to use try/catch.
|
|
||
| const readContent = await fs.promises.readFile(constructedPath, 'utf8'); | ||
|
|
||
| // TODO(bckenny): minify inlined javascript. |
There was a problem hiding this comment.
Is this part necessary? I think the subsequent steps of the bundler will handle the minification.
There was a problem hiding this comment.
this is for the inlined string, which the minification at the bundler level will ignore as just a string literal. We're manually running the current minifyFileTransform rather than passing it in, more or less
There was a problem hiding this comment.
doh! of course. sgtm
build/test/plugins/inline-fs-test.js
Outdated
| expect(code).toBe(`const myTextContent = "template literal text content";`); | ||
| }); | ||
|
|
||
| it('warns and skips unsupported syntax but inlines subsequent fs method calls', async () => { |
There was a problem hiding this comment.
This test asserts that the plugin won't attempt to replace the call using filePathVar, because it is not a string literal / is literally undefined here. When the plugin is changed to support statically-defined variables, this test case will then lose a little bit of clarity.
How about changing to something 100% invalid syntax, to explicitly test the robustness of the plugin? like
const myContent = fs.readFileSync(...!..., 'utf8');\nconst replacedContent = fs.readFileSync('${tmpPath}', 'utf8');
(or use some sufficiently hot-off-the-press JS language proposal that acorn does not yet support)
Could also add as a new test case, and just change the name of this test case to be "warns and skips fs method calls using unresolvable variables"
There was a problem hiding this comment.
yeah, I think maybe I meant "unsupported construct" or something? Really the specific error was less important than the fact that it recovers from an fs call it can't inline and keeps looking for more. I'll rename the test to be clearer.
(I also have a real unsupported syntax test for when collapseToStringLiteral is filled in to catch that case)
| expect(code).toBe(`const myContent = fs.readFileSync(filePathVar, 'utf8');\nconst replacedContent = "template literal text content";`); | ||
| }); | ||
|
|
||
| // TODO: this will work |
There was a problem hiding this comment.
meant to leave this comment?
There was a problem hiding this comment.
meant to leave this comment?
yeah, just a note that it's not able to handle it now but will replace __dirname when the full thing is implemented
types/acorn/index.d.ts
Outdated
| /** | ||
| * @fileoverview Types to provide basic `acorn` coverage, mainly to open up full | ||
| * ESTree AST types for the subset of acorn functionality we need. | ||
| * See https://github.com/acornjs/acorn/issues/946 |
There was a problem hiding this comment.
using the package from DefinitelyTyped is not sufficient?
There was a problem hiding this comment.
using the package from
DefinitelyTypedis not sufficient?
No, see that thread. The shipped acorn.d.ts takes precedence over the definitely typed version. However I think it's feasible to get rid of this file in exchange for two @ts-expect-errors in the parseExpressionAt function, so that's probably worth it
| const code = await inlineFs(content); | ||
| // eslint-disable-next-line max-len | ||
| expect(code).toBe(`const myContent = fs.readFileSync(filePathVar, 'utf8');\nconst replacedContent = "template literal text content";`); | ||
| }); |
There was a problem hiding this comment.
add a TODO to check for a warning (for once you've added that)
yes, just what I was thinking :) I was going to use more or less esbuild's version because they're nice and if we decide someday to switch... :) |
| const parser = new acorn.Parser(options, input, offset); | ||
| // @ts-expect-error - Not part of the current acorn types. | ||
| parser.nextToken(); | ||
| // @ts-expect-error - Not part of the current acorn types. |
There was a problem hiding this comment.
yeah, these aren't part of the current acorn.d.ts, but between the shipped types and using the estree Node type we don't have to maintain our own type file, which seems like a big win
part of #13231
To start, it only supports absolute string paths in
fs.readFileSyncandfs.readdirSync. It's not hooked up to anything but its tests.The approach is outlined in the code, but to recap:
codefor fs methods with regexstatically evaluate arguments to fs method, collapsing to single stringcurrently only allow string literalsnext will be more supported argument types, structured warnings, source maps, replacing the actual plugins, etc