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(zip): Force sources zip and auto source on opera #1014

Merged
merged 7 commits into from
Oct 2, 2024

Conversation

Timeraa
Copy link
Contributor

@Timeraa Timeraa commented Sep 30, 2024

Closes #1012

This pull request introduces new functionality to always create a sources zip file under certain conditions and updates related tests, configurations, and command-line options accordingly.

New Functionality:

  • Added a new option to always create a sources zip file, even when the browser is not Firefox or Opera. (packages/wxt/src/types.ts, packages/wxt/src/core/zip.ts, packages/wxt/src/core/resolve-config.ts, packages/wxt/src/cli/commands.ts) [1] [2] [3] [4] [5]

Tests:

  • Added new tests to verify the creation of sources zip files for Opera and under different configurations. (packages/wxt/e2e/tests/zip.test.ts)

Configuration:

  • Updated configuration handling to include the new zipSources option. (packages/wxt/src/types.ts, packages/wxt/src/core/utils/testing/fake-objects.ts) [1] [2]

Command-Line Interface:

  • Added a new CLI option --sources to always create sources zip files regardless of the browser. (packages/wxt/src/cli/commands.ts) [1] [2]

Copy link

netlify bot commented Sep 30, 2024

Deploy Preview for creative-fairy-df92c4 ready!

Name Link
🔨 Latest commit 45ac32c
🔍 Latest deploy log https://app.netlify.com/sites/creative-fairy-df92c4/deploys/66fd4f8b2532cd00086a8f26
😎 Deploy Preview https://deploy-preview-1014--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
Collaborator

@aklinker1 aklinker1 left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the PR. A few small changes, then we'll get this thing merged.

Comment on lines 57 to 59
wxt.config.browser === 'firefox' ||
wxt.config.browser === 'opera' ||
config?.zip?.alwaysBuildSourcesZip
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having this logic here, can we create a wxt.config.zip.zipSources boolean that gets set to true or false inside resolve-config.ts? That way this code can just be if (wxt.config.zip.zipSources) { ... }.

This is the pattern I've been following with all config - simplify config as much of it as possible inside resolve-config.ts so if it's referenced multiple times throughout the code, we don't have to recalculate the value multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, idk why but adding / changing something in the config is really hard? You have to manually add it in like 4 places and typecheck doesn't even throw an error if you forget it at one?

Copy link
Collaborator

@aklinker1 aklinker1 Oct 2, 2024

Choose a reason for hiding this comment

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

Hmm, types should throw an error... I guess unless you use optional types, then it won't. I try to use | undefined in the resolved config type to avoid that.

packages/wxt/src/types.ts Outdated Show resolved Hide resolved
Copy link

pkg-pr-new bot commented Oct 1, 2024

Open in Stackblitz

@wxt-dev/auto-icons

pnpm add https://pkg.pr.new/@wxt-dev/auto-icons@1014

@wxt-dev/i18n

pnpm add https://pkg.pr.new/@wxt-dev/i18n@1014

@wxt-dev/module-react

pnpm add https://pkg.pr.new/@wxt-dev/module-react@1014

@wxt-dev/module-solid

pnpm add https://pkg.pr.new/@wxt-dev/module-solid@1014

@wxt-dev/module-svelte

pnpm add https://pkg.pr.new/@wxt-dev/module-svelte@1014

@wxt-dev/module-vue

pnpm add https://pkg.pr.new/@wxt-dev/module-vue@1014

wxt

pnpm add https://pkg.pr.new/wxt@1014

commit: 45ac32c

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.98%. Comparing base (a64ff22) to head (45ac32c).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1014      +/-   ##
==========================================
- Coverage   82.14%   81.98%   -0.16%     
==========================================
  Files         127      127              
  Lines        6625     6633       +8     
  Branches     1103     1101       -2     
==========================================
- Hits         5442     5438       -4     
- Misses       1169     1181      +12     
  Partials       14       14              

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

Copy link
Collaborator

@aklinker1 aklinker1 left a comment

Choose a reason for hiding this comment

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

Alright, this looks good. I updated some of the docs, simplified some code, and added more tests. Gonna merge and release.

@aklinker1 aklinker1 merged commit d75f64d into wxt-dev:main Oct 2, 2024
18 checks passed
@Timeraa
Copy link
Contributor Author

Timeraa commented Oct 2, 2024

Looks good and oof I forgot to remove that console log haha

@avi12
Copy link

avi12 commented Oct 8, 2024

Any chance to produce different zip sources depending on the browser? Because a source to build a Firefox extension will be different than an Opera one

@aklinker1
Copy link
Collaborator

@avi12 https://wxt.dev/api/reference/wxt/interfaces/InlineConfig.html#sourcestemplate

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.

Pass CLI flag to force zip source creation
3 participants