Re-run ngsw after modifications in app-shell by filipesilva · Pull Request #11583 · angular/angular-cli

filipesilva · 2018-07-18T16:57:36Z

gkalpak

🎉

gkalpak · 2018-07-18T17:10:56Z

+    'ngsw-worker.js',
+    'worker-basic.min.js',
+    'safety-worker.js',
+  ];


This hard-coded list feels very fragile 😁

Those are the files we actually write back to disk further down on this file, so I think the knowledge of which files these are needs to be on the file anyway. Someone changing this file would have to change paths in both places though.

gkalpak · 2018-07-18T17:12:25Z

    return architect.getBuilderDescription(builderConfig).pipe(
      concatMap(description => architect.validateBuilderOptions(builderConfig, description)),
-      map(config => join(normalize(config.options.outputPath), 'index.html')),
+      // map(config => join(normalize(config.options.outputPath), 'index.html')),


Shouldn't this be removed?

gkalpak · 2018-07-18T17:15:12Z

          })
+          .then(() => {
+            if (browserOptions.serviceWorker) {
+              return augmentAppWithServiceWorker(


I wonder if re-running this would introduce a noticeable delay for large projects (with lots of files to hash).

That's a concern, yeah. I don't think ngsw has a way to just update the hashes for some files though. I don't want to duplicate the hashing logic it uses inside the CLI.

The logic to hash files and write ngsw.json leaves in angular/angular-cli fwiw (mostly here). It wouldn't be too difiicult to add a function that reads ngsw.json, updates some file hashes and writes it back.

But this is not a very robust solution imo.
The current solution is probably fine for now, but on the long run I would expect a way to schedule "post-build" tasks, which would run after all builders have finished building.

I think this is the bit that calculates hashes:

angular-cli/packages/angular_devkit/build_angular/src/angular-cli-files/utilities/service-worker/index.ts

Lines 135 to 140 in 8475b3d

switchMap(configJson => {

const Generator = require(swConfigPath).Generator;

const gen = new Generator(new CliFilesystem(host, outputPath), baseHref);

return gen.process(configJson);

}),

It lives in @angular/service-worker but we import it here to obtain the updated manifest, then write it back.

Generator calls CliFilessytem.list() to get the files and CliFilesystem.hash() to hash them. Then the ngsw.json manifest is written to dist further below. So, all the tools needed to read the existins ngsw.json, read and hash files and write the updated ngsw.json back are mostly in this file. (OK, there is a tiny piece of logic; i.e. how to update the hashtable in ngsw.json 😁)

Alternatively, Generator could expose a method to update specific files in an existing ngsw.json. So, then you could read the existing ngsw.json, pass it to Generator.updateHashes() and write the updated ngsw.json back.

But, I guess, that would mean that you need to check if the method is available (and this wouldn't work with older versions). Not sure how much of a problem this would be and whether it is worth it.

Oh wow, that is right. I hadn't noticed it.

@alxhub do you have a suggestion on what the best approach is? I think you were the one that wrote the original code.

I'm not sure this is worth optimizing just yet.

Honestly, the "correct" solution to me is to not generate ngsw.json until the entire build is done, including the App Shell. Running it twice seems like a hack (but a hack that will work).

alxhub

LGTM

I would favor a solution which only runs ngsw generation once, after all other build steps are completed. But this is an acceptable workaround if that's architecturally difficult right now.

alxhub · 2018-07-19T16:48:36Z



 class CliFilesystem implements Filesystem {
+  private _augmentedSwPaths = [


Maybe extract this to a const instead of a private field, to further indicate these are hardcoded paths.

alxhub · 2018-07-19T16:51:49Z

          })
+          .then(() => {
+            if (browserOptions.serviceWorker) {
+              return augmentAppWithServiceWorker(


I'm not sure this is worth optimizing just yet.

Honestly, the "correct" solution to me is to not generate ngsw.json until the entire build is done, including the App Shell. Running it twice seems like a hack (but a hack that will work).

… rendered

filipesilva · 2018-07-20T11:38:06Z

@gkalpak @alxhub took a slightly different approach: when running the browser target always override serviceWorker to false, and then after rendering universal augment with service worker if the browser target had service worker on.

This way it only runs once and there's no need to change the service worker code. PTAL when you have time.

gkalpak

💯

angular-automatic-lock-bot · 2019-09-12T12:40:58Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Jul 18, 2018

filipesilva added 2 commits July 18, 2018 17:58

test(@angular-devkit/build-angular): add app-shell route test

cd4fac5

test(@angular-devkit/build-angular): add failing app-shell + sw test

f29b962

filipesilva force-pushed the app-shell-sw branch 2 times, most recently from cbb62f6 to 55cca33 Compare July 18, 2018 17:04

gkalpak reviewed Jul 18, 2018

View reviewed changes

filipesilva force-pushed the app-shell-sw branch from 55cca33 to 9594ceb Compare July 18, 2018 17:36

filipesilva requested a review from alxhub July 18, 2018 20:48

alxhub previously approved these changes Jul 19, 2018

View reviewed changes

fix(@angular-devkit/build-angular): only add ngsw after universal has…

45d4971

… rendered

filipesilva dismissed alxhub’s stale review via 45d4971 July 20, 2018 11:36

filipesilva force-pushed the app-shell-sw branch from 9594ceb to 45d4971 Compare July 20, 2018 11:36

gkalpak approved these changes Jul 20, 2018

View reviewed changes

alxhub approved these changes Jul 20, 2018

View reviewed changes

Brocco approved these changes Jul 20, 2018

View reviewed changes

filipesilva merged commit 290929c into angular:master Jul 20, 2018

filipesilva deleted the app-shell-sw branch July 20, 2018 20:21

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 12, 2019

	switchMap(configJson => {
	const Generator = require(swConfigPath).Generator;
	const gen = new Generator(new CliFilesystem(host, outputPath), baseHref);

	return gen.process(configJson);
	}),



		class CliFilesystem implements Filesystem {
		private _augmentedSwPaths = [

Conversation

filipesilva commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipesilva commented Jul 20, 2018

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot Bot commented Sep 12, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

filipesilva commented Jul 18, 2018 •

edited

Loading