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

report(build): fix building flow report with inline-fs #13280

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

brendankenny
Copy link
Member

the change in #13275 to unguard the fs.readdirSync broke the flow report because it didn't have a call to inlineFs in the build call. This adds the plugin and fixes it.

This does add 466 bytes (the list of supported locales) to the flow report that aren't used. I assume there will be better DCE when switching to es modules and rollup will be more aggressive with tree shaking?

@brendankenny brendankenny requested a review from a team as a code owner October 28, 2021 22:43
@brendankenny brendankenny requested review from connorjclark and removed request for a team October 28, 2021 22:43
@google-cla google-cla bot added the cla: yes label Oct 28, 2021
@brendankenny brendankenny requested review from adamraine and removed request for connorjclark October 28, 2021 22:43
@@ -59,6 +59,7 @@ async function buildFlowReport() {
const bundle = await rollup.rollup({
input: 'flow-report/standalone-flow.tsx',
plugins: [
rollupPlugins.inlineFs({verbose: true}),
Copy link
Member

Choose a reason for hiding this comment

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

The files being read are not used by the flow report, so wouldn't this unnecessarily increase the size of the bundle?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's the 466 bytes I mentioned. It's not the files themselves, it's the locales file list. The format.js line ends up as

It=["ar-XB.json","ar.json","bg.json","ca.json","cs.json","da.json","de.json","el.json","en-GB.json","en-US.ctc.json","en-US.json","en-XA.json","en-XL.ctc.json","en-XL.json","es-419.json","es.json","fi.json","fil.json","fr.json","he.json","hi.json","hr.json","hu.json","id.json","it.json","ja.json","ko.json","lt.json","lv.json","nl.json","no.json","pl.json","pt-PT.json","pt.json","ro.json","ru.json","sk.json","sl.json","sr-Latn.json","sr.json","sv.json","ta.json","te.json","th.json","tr.json","uk.json","vi.json","zh-HK.json","zh-TW.json","zh.json"].filter((e=>e.endsWith(".json")&&!e.endsWith(".ctc.json"))).map((e=>e.replace(".json",""))).sort()

another option would be changing the rollup shim to something like 'fs': 'export function readdirSync() {return []}',. The downside would be it's more fragile to future additions of fs methods 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, this is fine then 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants