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

docs: Migrate Service Worker to Standalone #51687

Closed
wants to merge 1 commit into from

Conversation

wardbell
Copy link
Contributor

@wardbell wardbell commented Sep 7, 2023

Although there are several pages, only service-worker-getting-started.md has an example. That example code was migrated.

Worked around the few references to NgModule.

Other changes as follows:

Revised "Getting Started"

The original service worker example page (service-worker-getting-started.md) is full of holes! I don’t know how anyone got this working just by following this page

  • it doesn’t tell you where to get the example (and it isn’t generated by AIO)
  • it doesn’t tell you that ng serve doesn’t support service worker apps
  • it mentions http-server in passing but not what that is or where to get it.
  • several commands tell you to supply the “project-name” but (a) it doesn’t tell you how to find that name and (b) you never actually have to provide a name at all. That includes on the command that gets http-server going.

Hid section on ngsw-config tool

Hid the discussion of the ngsw-config tool in service-worker-config.md because (a) the command invoke it didn't seem to work and (b) it referenced <project-name> without explaining what that is or where to find it.

Left the hidden text in the page in case someone wants to do something about this.

Meanwhile, you seem to be able to edit the ngsw-config.json and then rebuild. That should suffice.

Disabled E2E

The AIO CI will no longer attempt to run the (thin) E2E tests.

These tests could lightly test the app with the dev server but couldn't test the Service Worker aspects; that would require launching the app with something other than ng serve and the AIO CI is not set up for that.

The existing tests appeared to pass when run locally with the dev server. However, only one of them actually ran; test runner reported. "There were tests whose specified size is too big."

Decided to bypass E2E testing of this project for now.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@ngbot ngbot bot added this to the Backlog milestone Sep 7, 2023
Although there are several pages, only `service-worker-getting-started.md` has an example.
That example code was migrated.

Worked around the few references to NgModule.

Other changes as follows:

**Revised "Getting Started"**

The original service worker example page (`service-worker-getting-started.md`) is full of holes! I don’t know how anyone got this working just by following this page

- it doesn’t tell you where to get the example (and it isn’t generated by AIO)
- it doesn’t tell you that ng serve doesn’t support service worker apps
- it mentions http-server in passing but not what that is or where to get it.
- several commands tell you to supply the “project-name” but (a) it doesn’t tell you how to find that name and (b) you never actually have to provide a name at all. That includes on the command that gets http-server going.

**Hid section on `ngsw-config` tool**

Hid the discussion of the `ngsw-config` tool in `service-worker-config.md` because
(a) the command invoke it didn't seem to work and (b) it referenced `<project-name>`
without explaining what that is or where to find it.

Left the hidden text in the page in case someone wants to do something about this.

Meanwhile, you seem to be able to edit the `ngsw-config.json` and then rebuild.
That should suffice.

**Disabled E2E**

The AIO CI will no longer attempt to run the (thin) E2E tests.

These tests could lightly test the app with the dev server but couldn't test the Service Worker aspects; that would require launching the app with something other than `ng serve` and the AIO CI is not set up for that.

The existing tests appeared to pass when run locally with the dev server.
However, only one of them actually ran; test runner reported.
"There were tests whose specified size is too big."

Decided to bypass E2E testing of this project for now.
@MarkTechson MarkTechson self-requested a review September 11, 2023 19:31
@MarkTechson MarkTechson added target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 11, 2023
Copy link
Contributor

@MarkTechson MarkTechson left a comment

Choose a reason for hiding this comment

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

@wardbell Can you please let me know about these couple of comments and then I think we can get this merged, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we need to delete this file?

Copy link
Contributor Author

@wardbell wardbell Sep 12, 2023

Choose a reason for hiding this comment

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

Because we want the CLI to create it dynamically; that's its job.
See https://angular.io/guide/service-worker-getting-started#adding-a-service-worker-to-your-project.

Creates the service worker configuration file called ngsw-config.json, which specifies the caching behaviors and other settings.

I stripped the example down to the state of an app prior to running the Service Worker CLI command. Before this change, the example was kind of half-way there with some (not all) of the CLI command artifacts.

I also added clear instructions on that guide page about what you should do if you choose to play along by downloading the example. Now that actually works (the example didn't work before and you had no clear idea what to do).

I might have kept it if any of the guide pages displayed it. But they don't.

@MarkTechson MarkTechson added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 13, 2023
Copy link
Contributor

@MarkTechson MarkTechson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@pkozlowski-opensource pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: merge The PR is ready for merge by the caretaker labels Sep 15, 2023
@MarkTechson MarkTechson added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 18, 2023
@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit 9f3b549.

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 20, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
Although there are several pages, only `service-worker-getting-started.md` has an example.
That example code was migrated.

Worked around the few references to NgModule.

Other changes as follows:

**Revised "Getting Started"**

The original service worker example page (`service-worker-getting-started.md`) is full of holes! I don’t know how anyone got this working just by following this page

- it doesn’t tell you where to get the example (and it isn’t generated by AIO)
- it doesn’t tell you that ng serve doesn’t support service worker apps
- it mentions http-server in passing but not what that is or where to get it.
- several commands tell you to supply the “project-name” but (a) it doesn’t tell you how to find that name and (b) you never actually have to provide a name at all. That includes on the command that gets http-server going.

**Hid section on `ngsw-config` tool**

Hid the discussion of the `ngsw-config` tool in `service-worker-config.md` because
(a) the command invoke it didn't seem to work and (b) it referenced `<project-name>`
without explaining what that is or where to find it.

Left the hidden text in the page in case someone wants to do something about this.

Meanwhile, you seem to be able to edit the `ngsw-config.json` and then rebuild.
That should suffice.

**Disabled E2E**

The AIO CI will no longer attempt to run the (thin) E2E tests.

These tests could lightly test the app with the dev server but couldn't test the Service Worker aspects; that would require launching the app with something other than `ng serve` and the AIO CI is not set up for that.

The existing tests appeared to pass when run locally with the dev server.
However, only one of them actually ran; test runner reported.
"There were tests whose specified size is too big."

Decided to bypass E2E testing of this project for now.

PR Close angular#51687
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants