Skip to content

core(build): inline-fs error if file missing, ignorePaths #14436

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 13, 2022
Merged

core(build): inline-fs error if file missing, ignorePaths #14436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e2526b6
build(inline-fs): fatal error if file missing, and ignore block comments
connorjclark Oct 6, 2022
d74e050
tests
connorjclark Oct 6, 2022
a0785d9
better
connorjclark Oct 6, 2022
482ee6f
test
connorjclark Oct 6, 2022
d9aa86b
ignore list
connorjclark Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion build/build-bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,12 @@ async function buildBundle(entryPath, distPath, opts = {minify: true}) {
}),
rollupPlugins.json(),
rollupPlugins.removeModuleDirCalls(),
rollupPlugins.inlineFs({verbose: false}),
rollupPlugins.inlineFs({
verbose: Boolean(process.env.DEBUG),
ignorePaths: [
require.resolve('puppeteer-core/lib/esm/puppeteer/common/Page.js'),
],
}),
rollupPlugins.commonjs({
// https://github.com/rollup/plugins/issues/922
ignoreGlobal: true,
Expand Down
2 changes: 1 addition & 1 deletion build/build-extension.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ async function buildEntryPoint() {
'___BROWSER_BRAND___': browserBrand,
}),
rollupPlugins.nodeResolve(),
rollupPlugins.inlineFs({verbose: false}),
rollupPlugins.inlineFs({verbose: Boolean(process.env.DEBUG)}),
rollupPlugins.terser(),
],
});
Expand Down
5 changes: 5 additions & 0 deletions build/plugins/inline-fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ async function inlineFs(code, filepath) {
parsed.callee.property);
}
} catch (err) {
// Consider missing files to be a fatal error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of weird that this isn't issuing more fatal errors, though I think this was copying the old brfs behavior of just warning on everything, and we rely on it for fs calls that aren't resolvable but also can't be reached at runtime.

I wonder if the rule should be something like: if there's an error statically determining the path, that's a warning, but if there's an fs error when accessing the path, that's an error, but we can change that if it's ever a bigger problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention was to only flag cases where fs.readFileSync failed, I'll double back and see how to really do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if you want to, I think this is fine, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alrighty. Let's just address it if we ever hit this case.

if (err.code === 'ENOENT') {
throw err;
}

// Use the specific node with the error if available; fallback to fs.method location.
const offsets = getNodeOffsets(err.node || parsed);
const location = acorn.getLineInfo(code, offsets.start);
Expand Down
7 changes: 6 additions & 1 deletion build/plugins/rollup-plugin-inline-fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {LH_ROOT} from '../../root.js';
/**
* @typedef Options
* @property {boolean} [verbose] If true, turns on verbose logging, e.g. log instances where fs methods could not be inlined.
* @property {string[]} [ignorePaths] Absoulte paths of files to not process for inlining.
*/

/**
Expand All @@ -29,9 +30,13 @@ function rollupInlineFs(options = {}) {
/**
* @param {string} originalCode
* @param {string} filepath
* @return {Promise<string|null>}
* @return {Promise<import('rollup').TransformResult>}
*/
async transform(originalCode, filepath) {
if (options.ignorePaths?.includes(filepath)) {
return;
}

// TODO(bckenny): add source maps, watch files.
const {code, warnings} = await inlineFs(originalCode, filepath);

Expand Down
5 changes: 5 additions & 0 deletions build/test/plugins/inline-fs-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,11 @@ describe('inline-fs', () => {
});
});

it('throws fatal error if file is missing', async () => {
const content = `const myTextContent = fs.readFileSync('i-never-exist.lol', 'utf8');`;
await expect(inlineFs(content, filepath)).rejects.toThrow('ENOENT');
});

it('inlines multiple fs.readFileSync calls', async () => {
fs.writeFileSync(tmpPath, 'some text content');
// eslint-disable-next-line max-len
Expand Down