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

Avoid some counter-productive lazy loaded files #7381

Merged
merged 9 commits into from
Jan 22, 2024
Merged

Conversation

fregante
Copy link
Contributor

What does this PR do?

// Dynamic import because documentView has a transitive dependency of react-shadow-root which assumed a proper
// `window` variable is present on module load. This isn't available on header generation
const DocumentView = React.lazy(
async () =>
import(
/* webpackChunkName: "components-lazy" */
"./DocumentView"
),
);

The comment is no longer applicable after the header script change:

This component was created in https://github.com/pixiebrix/pixiebrix-extension/pull/1999/files#diff-449c5467324895cc4e550d57294d9308482dbb34d09856db7d71ae5a73c58586

Checklist

  • Designate a primary reviewer: @mnholtz since you were working around this component now

@fregante
Copy link
Contributor Author

Feel free to close if there are other reasons to lazy-load this component

@fregante fregante changed the title Drop DocumentViewLazy component Drop DocumentViewLazy component and elementPicker lazyLoad Jan 20, 2024
@fregante fregante changed the title Drop DocumentViewLazy component and elementPicker lazyLoad Drop DocumentViewLazy component and elementPicker lazy load Jan 20, 2024
// Include here to avoid error during header generation (which runs in node environment)
const { userSelectElement } = await import(
/* webpackChunkName: "editorContentScript" */ "@/contentScript/pageEditor/elementPicker"
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also dropping this for the same reason. This is actually how I ended up on this comment, via awesome-webextension/webpack-target-webextension#42 (comment)

Copy link

codecov bot commented Jan 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7a5e2e9) 72.52% compared to head (b4642fa) 72.54%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7381      +/-   ##
==========================================
+ Coverage   72.52%   72.54%   +0.01%     
==========================================
  Files        1207     1205       -2     
  Lines       37684    37676       -8     
  Branches     7067     7067              
==========================================
  Hits        27331    27331              
+ Misses      10353    10345       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@twschiller
Copy link
Contributor

Feel free to close if there are other reasons to lazy-load this component

It's conceivably better from a sidebar startup time due to code-splitting. But you'd need to double-check the bundles before vs after

@fregante fregante changed the title Drop DocumentViewLazy component and elementPicker lazy load Avoid some counter-productive lazy loaded files Jan 22, 2024
@fregante
Copy link
Contributor Author

It's conceivably better from a sidebar startup time due to code-splitting. But you'd need to double-check the bundles before vs after

  • DocumentLazy: 63KB
  • editorContentScript: 20KB

Those are the minified and sourcemap-less sizes.

  • Execution: Since React doesn't actually execute that code until the component is picked up, it shouldn't make a different at runtime.
  • Loading/parsing: At 67K we might consider it a tiny loading-time win. I can undo DocumentLazy if you'd like.

This lead me to see that a lot of bundles are empty:

  • MarkdownLazy: 0.5KB
  • MarkdownInlineLazy: 0.5KB

I dropped these two because they shared the same components-lazy bundle name, which now sat at 1KB.

@grahamlangford
Copy link
Collaborator

Loading/parsing: At 67K we might consider it a tiny loading-time win. I can undo DocumentLazy if you'd like.

I think it's worth reverting DocumentLazy. But we should definitely keep your other changes.

Copy link

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

@fregante fregante merged commit 7014309 into main Jan 22, 2024
16 checks passed
@fregante fregante deleted the F/dev/DocumentViewLazy branch January 22, 2024 16:23
@grahamlangford grahamlangford added this to the 1.8.8 milestone Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants