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

fix: Exclude skipped entrypoints from Firefox sources zip #1238

Merged
merged 10 commits into from
Dec 6, 2024

Conversation

nishu-murmu
Copy link
Contributor

@nishu-murmu nishu-murmu commented Dec 2, 2024

@aklinker1 I've applied the logic as you mentioned, passing a boolean parameter to the findEntrypoints function.
Additionally while filtering options.exclude I had to use relative path, cause it wasn't working in minimatch method.

I've tested this on Windows.

This fixes #574

Copy link

netlify bot commented Dec 2, 2024

Deploy Preview for creative-fairy-df92c4 ready!

Name Link
🔨 Latest commit 4cd5547
🔍 Latest deploy log https://app.netlify.com/sites/creative-fairy-df92c4/deploys/675242d2acc4880008b135d7
😎 Deploy Preview https://deploy-preview-1238--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.

Thanks for the PR! However, as it is now, this will not work. See comments below.

packages/wxt/src/core/utils/building/find-entrypoints.ts Outdated Show resolved Hide resolved
packages/wxt/src/core/utils/building/internal-build.ts Outdated Show resolved Hide resolved
@aklinker1
Copy link
Collaborator

Actually, after looking at findEntrypoints a bit more, we should change it to return all entrypoints all the time. However, we need to make sure skipped is set properly based on if the entrypoint is excluded. Then, inside groupEntrypoints, we should only add entrypoints that aren't skipped to the groups to build.

That way, you can just call findEntrypoints inside zip.ts and update excludeSources accordingly, also in that file. I don't want to go modifying config in random places, I'd rather put the custom logic for excluding sources from being zipped right next to the code where we zip sources.

If we put it in the find-entrypoints.ts file like it is now, it's going to be a confusing side-effect that's hard to find in the future.

@aklinker1
Copy link
Collaborator

aklinker1 commented Dec 3, 2024

@nishu-murmu I created a PR into your fork with my suggested changes mentioned in the previous comment. I realized it probably didn't make much sense to you, so I did it myself.

nishu-murmu#1

Edit: I decided to make these changes it's own PR into WXT, so once #1244 is merged, we can update this branch like so:

// zip.ts
  if (wxt.config.zip.zipSources) {
+   const entrypoints = await findEntrypoints();
+   const skippedEntrypoints = entrypoints.filter(entry => entry.skipped);
+   const excludeSources = [...wxt.config.zip.excludeSources, ...skippedEntrypoints.map(entry => relative(wxt.config.zip.sourcesRoot, entry.inputPath))]
    await wxt.hooks.callHook('zip:sources:start', wxt);
    // ...
    await zipDir(wxt.config.zip.sourcesRoot, sourcesZipPath, {
      include: wxt.config.zip.includeSources,
-     exclude: wxt.config.zip.excludeSources,
+     exclude: excludeSources,

That should be all the changes this branch need.

@aklinker1 aklinker1 changed the title fix: filtered out excluded entryPoint files when pack into firefox source zip fix: Exclude skipped entrypoints from Firefox sources zip Dec 3, 2024
@nishu-murmu
Copy link
Contributor Author

I've refactored the code.
Now it seems to be working.

@aklinker1
Copy link
Collaborator

aklinker1 commented Dec 4, 2024

@nishu-murmu Could you add an e2e test for this in packages/wxt/e2e/tests/zip.test.ts?

See this test for reference:

it('should allow zipping hidden files into sources when explicitly listed', async () => {
const project = new TestProject({
name: 'test',
version: '1.0.0',
});
project.addFile(
'entrypoints/background.ts',
'export default defineBackground(() => {});',
);
project.addFile('.env');
project.addFile('.hidden-dir/file');
project.addFile('.hidden-dir/nested/file1');
project.addFile('.hidden-dir/nested/file2');
const unzipDir = project.resolvePath('.output/test-1.0.0-sources');
const sourcesZip = project.resolvePath('.output/test-1.0.0-sources.zip');
await project.zip({
browser: 'firefox',
zip: {
includeSources: ['.env', '.hidden-dir/file', '.hidden-dir/nested/**'],
},
});
await extract(sourcesZip, { dir: unzipDir });
expect(await project.fileExists(unzipDir, '.env')).toBe(true);
expect(await project.fileExists(unzipDir, '.hidden-dir/file')).toBe(true);
expect(await project.fileExists(unzipDir, '.hidden-dir/nested/file1')).toBe(
true,
);
expect(await project.fileExists(unzipDir, '.hidden-dir/nested/file2')).toBe(
true,
);
});

You can run the test via:

cd packages/wxt
pnpm test zip.test.ts

Then I'll approve this and we can get it released!

@nishu-murmu
Copy link
Contributor Author

@nishu-murmu I created a PR into your fork with my suggested changes mentioned in the previous comment. I realized it probably didn't make much sense to you, so I did it myself.

nishu-murmu#1

Edit: I decided to make these changes it's own PR into WXT, so once #1244 is merged, we can update this branch like so:

// zip.ts
  if (wxt.config.zip.zipSources) {
+   const entrypoints = await findEntrypoints();
+   const skippedEntrypoints = entrypoints.filter(entry => entry.skipped);
+   const excludeSources = [...wxt.config.zip.excludeSources, ...skippedEntrypoints.map(entry => relative(wxt.config.zip.sourcesRoot, entry.inputPath))]
    await wxt.hooks.callHook('zip:sources:start', wxt);
    // ...
    await zipDir(wxt.config.zip.sourcesRoot, sourcesZipPath, {
      include: wxt.config.zip.includeSources,
-     exclude: wxt.config.zip.excludeSources,
+     exclude: excludeSources,

That should be all the changes this branch need.

const entrypoints = await findEntrypoints();
This line is causing problem

Cause in the zip.ts file

const output = await internalBuild();
is being called which is internally calling findEntrypoints()

then
const entrypoints = await findEntrypoints(); in line number 58 is calling again.

This is causing:
this below error:
AssertionError: Expected "entrypoints:resolved" to be called 1 time(s): expected "spy" to be called 1 times, but got 2 times.

Let me see what I can do.

@aklinker1
Copy link
Collaborator

@nishu-murmu I would recomend just updating the test to expect the hook to be called twice. That should be fine

@nishu-murmu
Copy link
Contributor Author

@nishu-murmu I would recomend just updating the test to expect the hook to be called twice. That should be fine

Yeah, I got it.

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.

🎉 :shipit:

Copy link

pkg-pr-new bot commented Dec 6, 2024

Open in Stackblitz

@wxt-dev/auto-icons

npm i https://pkg.pr.new/@wxt-dev/auto-icons@1238

@wxt-dev/module-react

npm i https://pkg.pr.new/@wxt-dev/module-react@1238

@wxt-dev/i18n

npm i https://pkg.pr.new/@wxt-dev/i18n@1238

@wxt-dev/module-solid

npm i https://pkg.pr.new/@wxt-dev/module-solid@1238

@wxt-dev/module-vue

npm i https://pkg.pr.new/@wxt-dev/module-vue@1238

@wxt-dev/module-svelte

npm i https://pkg.pr.new/@wxt-dev/module-svelte@1238

@wxt-dev/storage

npm i https://pkg.pr.new/@wxt-dev/storage@1238

@wxt-dev/unocss

npm i https://pkg.pr.new/@wxt-dev/unocss@1238

wxt

npm i https://pkg.pr.new/wxt@1238

commit: 7439390

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.83%. Comparing base (fd436a0) to head (7439390).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1238      +/-   ##
==========================================
- Coverage   81.03%   80.83%   -0.21%     
==========================================
  Files         128      128              
  Lines        6123     6136      +13     
  Branches     1038     1041       +3     
==========================================
- Hits         4962     4960       -2     
- Misses       1146     1161      +15     
  Partials       15       15              

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

@aklinker1 aklinker1 merged commit e221252 into wxt-dev:main Dec 6, 2024
4 checks passed
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.

filtered out excluded entryPoint files when pack into firefox source zip
2 participants