-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
PWA template #18878
PWA template #18878
Conversation
...orWasm.ProjectTemplates/content/BlazorWasm-CSharp/Client/wwwroot/service-worker.published.js
Outdated
Show resolved
Hide resolved
src/ProjectTemplates/BlazorWasm.ProjectTemplates/BlazorWasm-CSharp.Client.csproj.in
Outdated
Show resolved
Hide resolved
...s/BlazorWasm.ProjectTemplates/content/BlazorWasm-CSharp/.template.config/dotnetcli.host.json
Show resolved
Hide resolved
src/ProjectTemplates/BlazorWasm.ProjectTemplates/BlazorWasm-CSharp.Client.csproj.in
Outdated
Show resolved
Hide resolved
<ServiceWorkerMoveFrom>$(PublishDir)$(BlazorPublishDistDir)service-worker.published.js</ServiceWorkerMoveFrom> | ||
<ServiceWorkerMoveTo>$(PublishDir)$(BlazorPublishDistDir)service-worker.js</ServiceWorkerMoveTo> | ||
</PropertyGroup> | ||
<Move SourceFiles="$(ServiceWorkerMoveFrom)" DestinationFiles="$(ServiceWorkerMoveTo)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not publish it without the published.js
extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do, that's what this does. Or are you suggesting this should be implemented differently?
I've tried to keep the MSBuild logic here incredibly basic. For example, not assuming the developer even knows what an item group is. It's just properties, which should be familiar to any OO developer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that I agree with Pranav, I feel it is better to do this outside of the user project. I don't think that the logic here is simple. We introduce the user to several MSBuild concepts and properties and it is not straightforward to understand what is happening here.
We could do this in several different ways:
- Add a new package for doing service workers.
- All the logic gets encapsulated there and we trigger it based on the presence of a service-worker.js file on your app.
- The fact that you added the package is the gesture you want the SW.
- Everything gets handled transparently for you.
- We put the logic inside blazor.build
- Same deal, but no package.
- We would need a property to turn it off
- Would be turned on by default based on the presence of a given file.
8e6d120
to
a7177ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked at the tests and other stuff yet, but I think that we could find an approach that strikes a better balance between simplicy and making our customers successful.
The reason I think that is the case, is that it makes it really hard for customers to be successful once their project grows beyond the most basic requirements, and while customers can use other toolchains to do very sophisticaded stuff to get their SW code generated automatically, I feel it is too complex of a choice to say just use webpack or something similar.
I believe that we should strive for a middle ground that leverages our strengths and doesn't set up our customers for failure once they go beyond the basics.
My concrete proposal involves:
- Have a single service-worker.js file.
- Produce a separate file with the list of all the resources and options for the service worker in a script form.
- Can be json, can be JS with an exported object.
- Can include just the list of resources or a more future proof config object that can be passed down to the SW.
- Filter the resources based on the list included in the service worker.
- Intercept fetch requests based on the options imported.
- We can generate both files at build time and automatically add the appropriate Content items to the build for you.
- You get to configure your service worker however you want.
- We simply give you facilities to leverage the existing knowledge we already have during the build.
- For example, other SW implementations can leverage this by importing the script/json file with this data.
- We simply give you facilities to leverage the existing knowledge we already have during the build.
...orWasm.ProjectTemplates/content/BlazorWasm-CSharp/Client/wwwroot/service-worker.published.js
Outdated
Show resolved
Hide resolved
src/ProjectTemplates/BlazorWasm.ProjectTemplates/BlazorWasm-CSharp.Client.csproj.in
Outdated
Show resolved
Hide resolved
<ServiceWorkerMoveFrom>$(PublishDir)$(BlazorPublishDistDir)service-worker.published.js</ServiceWorkerMoveFrom> | ||
<ServiceWorkerMoveTo>$(PublishDir)$(BlazorPublishDistDir)service-worker.js</ServiceWorkerMoveTo> | ||
</PropertyGroup> | ||
<Move SourceFiles="$(ServiceWorkerMoveFrom)" DestinationFiles="$(ServiceWorkerMoveTo)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that I agree with Pranav, I feel it is better to do this outside of the user project. I don't think that the logic here is simple. We introduce the user to several MSBuild concepts and properties and it is not straightforward to understand what is happening here.
We could do this in several different ways:
- Add a new package for doing service workers.
- All the logic gets encapsulated there and we trigger it based on the presence of a service-worker.js file on your app.
- The fact that you added the package is the gesture you want the SW.
- Everything gets handled transparently for you.
- We put the logic inside blazor.build
- Same deal, but no package.
- We would need a property to turn it off
- Would be turned on by default based on the presence of a given file.
src/ProjectTemplates/BlazorWasm.ProjectTemplates/BlazorWasm-CSharp.Client.csproj.in
Outdated
Show resolved
Hide resolved
…ustomize the list
...orWasm.ProjectTemplates/content/BlazorWasm-CSharp/Client/wwwroot/service-worker.published.js
Show resolved
Hide resolved
src/ProjectTemplates/BlazorWasm.ProjectTemplates/BlazorWasm-CSharp.Client.csproj.in
Show resolved
Hide resolved
...orWasm.ProjectTemplates/content/BlazorWasm-CSharp/Client/wwwroot/service-worker.published.js
Show resolved
Hide resolved
@@ -24,6 +25,11 @@ public BlazorWasmTemplateTest(ProjectFactoryFixture projectFactory, BrowserFixtu | |||
|
|||
public ProjectFactoryFixture ProjectFactory { get; set; } | |||
|
|||
public override Task InitializeAsync() | |||
{ | |||
return InitializeAsync(isolationContext: Guid.NewGuid().ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Output.WriteLine($"Opening browser without corresponding server at {listeningUri}..."); | ||
Browser.Navigate().GoToUrl(listeningUri); | ||
TestBasicNavigation(project.ProjectName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is D O P E
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super dope. I like a lot how it turned out.
Great changes!
The core idea this design is based on is that we want to minimize ties that Blazor has to any specific approach to service workers or offline support. This is because:
So, the approach used in this PR is to simplify things down to a really minimal, safe implementation that can exist entirely within the template. We're not shipping service worker features; we're just shipping an example of one way to do it in the template. Anyone who wants to do things a different way can do so, using whatever APIs or 3rd-party tooling they want, because there's literally nothing Blazor-specific in the requirements. Getting to this point of "no Blazor-specific requirements" is the whole point of doing #18859, which entirely removes .NET assemblies and linking from the picture as far as offline support is concerned.Update: After discussions, we decided that we do want to ship some serviceworker-related build tooling. Specifically, we want to generate on build a manifest of the static files being published. Then a service worker can read this manifest and use its own logic to decide which of those files to actually cache for offline use.
#18859 is no longer a prerequisite for this work, but since it now exists and gives a significant measurable improvement to load times when not using a service worker, we should keep that anyway as an independent piece. It doesn't conflict with what we have here.
Offline strategy
There are many different ways to do offline. The biggest choice is between "network first" and "offline first" strategies. A "network first" approach always tries to get updates from the network, and falls back on cached data if the network is unavailable. A "cache first" approach always uses content from the cache, and separately updates the cache in the background at certain times.
The safest way to do offline is "cache first", so that's what we do here. It's also the same choice taken by Create-React-App's PWA mode and Polymer's PWA Starter Kit. The reason is:
The specific mechanism for detecting service worker changes, creating a new cache, populating it, and only switching over once it's fully populated without errors, is a pattern designed into the basic service worker APIs (the "install"/"activate" cycle). There are awkward issues with it:
We will need to document these caveats. Despite these issues, this is the most standard way to build a service worker, and is the same as other major templates/starter kits do.
I strongly considered putting more complex logic into the service worker to mitigate one or both of these (for example, offline-first, but even before that try asking the network to check for updates, and if we find one then switch to network-first). However there are too many reasons why doing nonstandard things would blow up in our faces, so I think it's better to focus on ensuring developers can put in any custom service worker logic of their own.
With the implementation here, offline support is only enabled for published apps. It doesn't make sense to try doing offline support during development because:
wwwroot
(or SWA inwwwroot
in reference projects) and reload, the offline-first strategy would have no way to know there are any changes to fetch. And even if it did, the hash checks would fail until you next build and regenerate the manifest.Developers who want to do their own different thing can do whatever they want. For example, they could implement a network-first strategy with no hash checking, in which case it would be fine to enable that in development (but would have lots more caveats in production).