core(build): add basic inline-fs plugin #13232

brendankenny · 2021-10-20T00:03:08Z

To start, it only supports absolute string paths in fs.readFileSync and fs.readdirSync. It's not hooked up to anything but its tests.

The approach is outlined in the code, but to recap:

scan code for fs methods with regex
parse only the expression at each found index
~~statically evaluate arguments to fs method, collapsing to single string~~ currently only allow string literals
execute fs method with computed argument
replace original expression with result of fs call
if an expression cannot be parsed or statically evaluated, warn and skip
if no expressions found or all are skipped, return null

next will be more supported argument types, structured warnings, source maps, replacing the actual plugins, etc

connorjclark

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 foundIndices to a method and try/catching over it, collecting errors from parsing as they happen
inlineFs could then return Promise<{code: string|null, warnings: string[]}> (or warnings?: 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.

connorjclark · 2021-10-20T18:42:27Z

build/plugins/inline-fs.js

+
+  const readContent = await fs.promises.readFile(constructedPath, 'utf8');
+
+  // TODO(bckenny): minify inlined javascript.


Is this part necessary? I think the subsequent steps of the bundler will handle the minification.

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

doh! of course. sgtm

connorjclark · 2021-10-20T18:51:13Z

build/test/plugins/inline-fs-test.js

+      expect(code).toBe(`const myTextContent = "template literal text content";`);
+    });
+
+    it('warns and skips unsupported syntax but inlines subsequent fs method calls', async () => {


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"

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)

connorjclark · 2021-10-20T18:52:54Z

build/test/plugins/inline-fs-test.js

+      expect(code).toBe(`const myContent = fs.readFileSync(filePathVar, 'utf8');\nconst replacedContent = "template literal text content";`);
+    });
+
+    // TODO: this will work


meant to leave 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

connorjclark · 2021-10-20T18:55:29Z

types/acorn/index.d.ts

+/**
+ * @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


using the package from DefinitelyTyped is not sufficient?

using the package from DefinitelyTyped is 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

connorjclark · 2021-10-20T18:57:02Z

build/test/plugins/inline-fs-test.js

+      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";`);
+    });


add a TODO to check for a warning (for once you've added that)

brendankenny · 2021-10-20T22:09:20Z

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.

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... :)

brendankenny · 2021-10-20T23:10:00Z

build/plugins/inline-fs.js

+  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.


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

core(build): add basic inline-fs plugin

bfae528

brendankenny requested a review from a team as a code owner October 20, 2021 00:03

brendankenny requested review from adamraine and removed request for a team October 20, 2021 00:03

google-cla bot added the cla: yes label Oct 20, 2021

devtools-bot assigned adamraine Oct 20, 2021

devtools-bot added the waiting4reviewer label Oct 20, 2021

connorjclark requested review from connorjclark and removed request for adamraine October 20, 2021 18:31

connorjclark assigned connorjclark and unassigned adamraine Oct 20, 2021

connorjclark approved these changes Oct 20, 2021

View reviewed changes

feedback

9a8879e

vercel bot deployed to Preview October 20, 2021 23:08 View deployment

brendankenny commented Oct 20, 2021

View reviewed changes

brendankenny added the land-when-ci-is-green label Oct 20, 2021

devtools-bot merged commit b592c39 into master Oct 20, 2021

devtools-bot deleted the inline-fs branch October 20, 2021 23:30

devtools-bot removed the land-when-ci-is-green label Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(build): add basic inline-fs plugin #13232

core(build): add basic inline-fs plugin #13232

brendankenny commented Oct 20, 2021

connorjclark left a comment

connorjclark Oct 20, 2021

brendankenny Oct 20, 2021

connorjclark Oct 20, 2021

connorjclark Oct 20, 2021

brendankenny Oct 20, 2021

connorjclark Oct 20, 2021

brendankenny Oct 20, 2021

connorjclark Oct 20, 2021

brendankenny Oct 20, 2021

connorjclark Oct 20, 2021

brendankenny commented Oct 20, 2021

brendankenny Oct 20, 2021


		const readContent = await fs.promises.readFile(constructedPath, 'utf8');

		// TODO(bckenny): minify inlined javascript.

core(build): add basic inline-fs plugin #13232

core(build): add basic inline-fs plugin #13232

Conversation

brendankenny commented Oct 20, 2021

connorjclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Oct 20, 2021

Choose a reason for hiding this comment