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

Add SW files exclusion support #1645

Merged
merged 12 commits into from
Jul 7, 2021

Conversation

hariombalhara
Copy link
Contributor

@hariombalhara hariombalhara commented Jun 7, 2021

Before submitting the PR, please make sure you do the following

Suggested Fix for Issue: #1636

It supports files exclusions using glob patterns from $service-worker.files
This exclusion can also be helpful when you have certain files present due to any of the following reason

  • You are experimenting with something
  • Someone just forgot to remove the file but he removed the actual usage.
  • Open Graph meta tags using an image that isn't used on the website directly but is served from the website when requested.

It allows you to control what you want to be cached during ServiceWorker installation, so installation time won't increase as your app grow.

I would add tests for the case once I get to know if I am going in the right direction

Tests

  • Unit Tests have been fixed and are passing. Integration tests are somehow failing for me for the master branch as well.

Changesets

Would generate a changelog later.

@benmccann
Copy link
Member

This would need some documentation

@hariombalhara
Copy link
Contributor Author

I have added the comments where I felt are justified.
Let me know if you meant that only or is there something else missing?

@benmccann
Copy link
Member

I meant to add something in https://github.com/sveltejs/kit/tree/master/documentation/docs so people know this option exists and how to use it

@hariombalhara
Copy link
Contributor Author

@benmccann Oops. I thought you were talking about this only 😄
Updated the documentation now.

@hariombalhara
Copy link
Contributor Author

@benmccann please look into it.

@benmccann
Copy link
Member

@hariombalhara it looks like this PR will need to be rebased

@hariombalhara
Copy link
Contributor Author

Sure will do it today

@changeset-bot
Copy link

changeset-bot bot commented Jun 16, 2021

🦋 Changeset detected

Latest commit: d885066

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@hariombalhara
Copy link
Contributor Author

There are no conflicts now.

@hariombalhara
Copy link
Contributor Author

@benmccann This is now sorted. I forgot to tag you.

@hariombalhara
Copy link
Contributor Author

@benmccann @ignatiusmb I am done with all changes.

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

lgtm. though I might want Rich or another maintainer to take a look since this one is adding new API surface via a new option

@benmccann
Copy link
Member

lint is failing. you'll need to fix that

@hariombalhara
Copy link
Contributor Author

hariombalhara commented Jun 21, 2021

lint is failing. you'll need to fix that

This is fixed in #1725. Unrelated to this PR.

lgtm. though I might want Rich or another maintainer to take a look since this one is adding new API surface via a new option

Sure.

@hariombalhara
Copy link
Contributor Author

@benmccann Lint should be passing now.

@hariombalhara
Copy link
Contributor Author

@Rich-Harris @benmccann Please take a look into this.
Happy to do any more changes if required.

@hariombalhara
Copy link
Contributor Author

@benmccann Please tell me the way forward on it.

hariombalhara and others added 2 commits July 7, 2021 08:55
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@benmccann benmccann added the feature request New feature or request label Jul 7, 2021
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

lgtm. Can you add a changeset by running pnpx changeset so that I can merge this?

packages/kit/src/core/create_manifest_data/index.js Outdated Show resolved Hide resolved
@hariombalhara
Copy link
Contributor Author

Added changeset

@benmccann benmccann merged commit 64f749d into sveltejs:master Jul 7, 2021
@hariombalhara
Copy link
Contributor Author

Thanks for reviewing and merging @benmccann!

sidharthv96 added a commit to sidharthv96/kit that referenced this pull request Jul 11, 2021
* 'master' of github.com:sidharthv96/kit: (1114 commits)
  Version Packages (next) (sveltejs#1858)
  Bump vite-plugin-svelte to 1.0.0-next.12 (sveltejs#1869)
  [fix] preserve user defined config and files on `svelte-kit package` (sveltejs#1735)
  [fix] handle undefined body on endpoint output (sveltejs#1808)
  [fix] copy essentials files from root on packaging (sveltejs#1747)
  [docs] sort config alphabetically (sveltejs#1867)
  add config.kit.package.emitTypes option (sveltejs#1852)
  [fix] add $lib alias to js/tsconfig (sveltejs#1860)
  Pass along custom properties added to Error (sveltejs#1821)
  Version Packages (next) (sveltejs#1840)
  Improve grammar in packages FAQ
  Docs for writing an adapter (sveltejs#1846)
  Additional documentation around pnpx changeset usage
  [feat] expose Vite.js `mode` from `$app/env` (sveltejs#1789)
  Service worker files exclusion support (sveltejs#1645)
  chore: Enable `vite.server.fs.strict` internally by default (sveltejs#1842)
  Test with the latest version of Svelte (sveltejs#1848)
  [docs] don't need to run pnpm install twice
  Improve HN example docs
  [fix] correct `ReadOnlyFormData` generator implementation (sveltejs#1837)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants