Skip to content
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

[tracking] replace brfs #13231

Open
brendankenny opened this issue Oct 19, 2021 · 0 comments
Open

[tracking] replace brfs #13231

brendankenny opened this issue Oct 19, 2021 · 0 comments
Assignees
Labels

Comments

@brendankenny
Copy link
Member

brendankenny commented Oct 19, 2021

We've used brfs for a very long time (introduced in #225), and have gone through a number of cycles of updating it, waiting for it to be updated, and rolling our own slightly tweaked versions. However we're moving on from browserify due to the move to ESM and the desire to get away with the difficulties of such an old toolchain (e.g. #12134) and it's similarly time to make a clean break from brfs.

There aren't a lot of drop-in replacements out there and it's not a very complicated transform, so we're just going to write out own.

Goals:

  • replace only fs.readFileSync and fs.readdirSync calls with statically evaluatable arguments
  • support rollup and browserify (while transitioning)
  • commonjs and module support
  • minify inlined JS
  • easy to update statically evaluated features (e.g. add LH_ROOT support)
  • better debuggability and ability to see exactly what was replaced and what was skipped (and why) without firing up a debugger
  • resilience to JS language changes by keeping code parsed to a minimum
    • e.g. the version of acorn used might not support private-static-class-member-coalesced-decorators like we'll be using in node 37, but there's no reason for the brfs replacement to care if that syntax isn't used within the fs method call being replaced. Since the types of syntax used to make the arguments to the fs calls largely hasn't changed in LH since maybe rest parameters were introduced, this should eliminate a large number of the upgrade pains we've had where syntax anywhere in the file has broken browserify and/or brfs

Non-goals

  • verify that the fs.readFileSync call is actually a method on the Node fs API and not some local variable fs = {readFileSync: () => true} or whatever.
    • verification would require parsing at least the global and surrounding scope of the call, conflicting with the last goal above. I don't see this ever being an actual issue (pick a better variable name regardless), but open to discussing if others disagree
  • spinning off into a separate module
    • as long as the implementation is relatively small and tested there's not much harm living in Lighthouse, and it lets us avoid adding config to support our use cases like LH_ROOT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants