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

feat(analysis): Open stats.html file automatically #564

Merged
merged 5 commits into from
Mar 24, 2024
Merged

Conversation

aklinker1
Copy link
Collaborator

@aklinker1 aklinker1 commented Mar 24, 2024

This closes #561.

Either set analysis.open in wxt.config.ts or run wxt build --analyze --analyze-open.

Todo

  • Don't open browser in CI

Copy link

netlify bot commented Mar 24, 2024

Deploy Preview for creative-fairy-df92c4 ready!

Name Link
🔨 Latest commit a74877c
🔍 Latest deploy log https://app.netlify.com/sites/creative-fairy-df92c4/deploys/66004e505c8b58000893826a
😎 Deploy Preview https://deploy-preview-564--creative-fairy-df92c4.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Mar 24, 2024

Codecov Report

Attention: Patch coverage is 96.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 86.32%. Comparing base (08f32aa) to head (a74877c).

Files Patch % Lines
src/core/utils/building/internal-build.ts 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #564      +/-   ##
==========================================
- Coverage   86.38%   86.32%   -0.07%     
==========================================
  Files         111      111              
  Lines        8874     8898      +24     
  Branches      877      877              
==========================================
+ Hits         7666     7681      +15     
- Misses       1194     1203       +9     
  Partials       14       14              

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

@aklinker1 aklinker1 marked this pull request as ready for review March 24, 2024 16:03
@aklinker1 aklinker1 merged commit 36b399d into main Mar 24, 2024
17 checks passed
@aklinker1 aklinker1 deleted the analysis-open branch March 24, 2024 16:05
Comment on lines -69 to +75
analysis: flags.analyze ? { enabled: true } : undefined,
analysis: flags.analyze
? {
enabled: true,
open: flags.analyzeOpen,
}
: undefined,

Choose a reason for hiding this comment

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

@aklinker1 how hard would it be to respect analysis.open from the config file if --analyzeOpen isn't passed?

Copy link
Collaborator Author

@aklinker1 aklinker1 Mar 24, 2024

Choose a reason for hiding this comment

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

It already behaves that way after this PR. The CLI flags override the relevant options in the config file

I would recommend setting the value in the config file if you always want to open it.

Choose a reason for hiding this comment

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

@aklinker1 this is not how things are behaving for me locally (on 0.17.8) - to clarify, my config looks like this:

analysis: { outputFile: "build/stats.html", open: true }

and when I run wxt build --analyze, analysis.outputFile is respected, but analysis.open is not.

FYI, I don't specify analysis.enable in my config as I only want to do this in a specific NPM script.

Could it be that the override logic isn't taking into account that flags.analyzeOpen is undefined, but analysis.open still exists if flags.analyze does (like how "prop" in { prop: undefined } is true)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting, let me take a look. You're setup is what I intended, so I'll fix it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, the undefined was overriding the true:

Screenshot 2024-03-25 at 12 11 08 AM

userConfig comes from the wxt.config.ts file, inlineConfig is the CLI flags, and merged is the output of merging them together.

Caused by the spread operator, replaced with defu and it's working: #567

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@joealden Released fix in v0.17.9

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

Successfully merging this pull request may close these issues.

Ability to Auto Open Browser with --analyze Build Flag
2 participants