core(build): inline-fs error if file missing, ignorePaths #14436
Conversation
connorjclark
changed the title
build/plugins/inline-fs.js
Outdated
| } | ||
| if (maybeInBlockComment) { | ||
| const err = new Error('ignoring potential match because likely inside a block comment'); | ||
| const line = code.substring(0, foundIndex).split(/\r\n|\r|\n/).length; |
There was a problem hiding this comment.
acorn.getLineInfo(code, foundIndex) is essentially this but lets you skip the DIY part (see use on L124)
| @@ -98,6 +114,11 @@ async function inlineFs(code, filepath) { | |||
| parsed.callee.property); | |||
| } | |||
| } catch (err) { | |||
| // Consider missing files to be a fatal error. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
My intention was to only flag cases where fs.readFileSync failed, I'll double back and see how to really do that.
There was a problem hiding this comment.
Only if you want to, I think this is fine, too.
There was a problem hiding this comment.
Alrighty. Let's just address it if we ever hit this case.
build/plugins/inline-fs.js
Outdated
| // First iterate backwards until a newline to determine if we are in a block comment. | ||
| let maybeInBlockComment = false; | ||
| for (let i = foundIndex; i > 0; i--) { | ||
| if (code[i] === '*') { |
There was a problem hiding this comment.
I'm just realizing this is horribly tailored to the one case I found. Doesn't catch all block comments (don't need an asterisk on every line...), and trips up on multiplication. But how can we do something similar w/o parsing the AST?
There was a problem hiding this comment.
We could drop trying to detect if inside a comment, go back to this being a warning, and iterate all warnings and throw on any ENOENT that isn't one we expect.
There was a problem hiding this comment.
This entire file seems tailored to our use cases. If we aren't handling one of these cases properly, will this plugin fail loudly?
There was a problem hiding this comment.
Yes, it'd be throwing an error and failing the build step. I'd just expect this to possibly get triggered upon some random node_modules change.
connorjclark
changed the title
connorjclark
changed the title
I noticed this when I tried to run
yarn build-lrin a clean checkout, which silently messes up because there is nodist/reportso the JS inlining fails by simply emitting code that tried to read the file withfs. Instead, the plugin should throw a fatal error.In doing this, I had to skip a case the plugin came across in
node_modules/puppeteer-core/lib/esm/puppeteer/common/Page.js: