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

feat(service-worker): add JSON schema for service worker config #27859

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@cyrilletuzi
Copy link
Contributor

cyrilletuzi commented Dec 28, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

No assistance/validation in editor for service worker config.

Issue Number: Fixes #19847

With angular/angular-cli#13325, it will also fix angular/angular-cli#13324

What is the new behavior?

Assistance/validation in editor for service worker config.

Does this PR introduce a breaking change?

  • No

Other information

The schema follows @angular/service-worker interfaces (here).

Descriptions come from the official documentation on angular.io (here).

Default values have been verified in @angular/service-worker code (here).

I don't understand how the build works. When following the contributing guidelines, I can't see the static files that should be copied (like safety-worker.js and schema.json) in dist builds. So this part have to be checked.

As this is just an helper for development, this could be added in a patch version.

@gkalpak

@gkalpak
Copy link
Member

gkalpak left a comment

Awesome work 💯

A couple of comments:

  1. Wrap property names/values in backticks or quotes (to make it clear you are referring to a property name/value).
  2. Change the commit message to match our guidelines; e.g.:

    feat(service-worker): add JSON schema for service worker config

    Fixes #19847

Show resolved Hide resolved packages/service-worker/config/test/generator_spec.ts Outdated
Show resolved Hide resolved packages/service-worker/BUILD.bazel Outdated
Show resolved Hide resolved packages/service-worker/schema.json Outdated
Show resolved Hide resolved packages/service-worker/schema.json Outdated
Show resolved Hide resolved packages/service-worker/schema.json Outdated
Show resolved Hide resolved packages/service-worker/schema.json Outdated
Show resolved Hide resolved packages/service-worker/schema.json Outdated
Show resolved Hide resolved packages/service-worker/config/src/in.ts Outdated
@cyrilletuzi

This comment has been minimized.

Copy link
Contributor Author

cyrilletuzi commented Dec 29, 2018

For the commit message, I don't see what I'm not respecting from the contributing guidelines, it's just adding the "fixes" part right?

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Dec 29, 2018

It's the "fixes" part and adding add (😉) to the subject (i.e. describing what change you are making: adding a JSON schema (in contrast to updating or fixing or refactoring the JSON schema)).

@cyrilletuzi cyrilletuzi force-pushed the cyrilletuzi:swschema branch from d03afcc to cffffe0 Dec 29, 2018

@cyrilletuzi cyrilletuzi changed the title feat(service-worker): JSON schema for service worker config feat(service-worker): add JSON schema for service worker config Dec 29, 2018

@cyrilletuzi

This comment has been minimized.

Copy link
Contributor Author

cyrilletuzi commented Dec 29, 2018

@gkalpak Changes done.

The bazel part (for copying the schema.json in the dist package) should be checked, as I don't know how to check this myself.

Should the fix of the doc (about the bad default for version) be part of this PR or be another?

@cyrilletuzi cyrilletuzi force-pushed the cyrilletuzi:swschema branch from d688b94 to e43cbc3 Dec 29, 2018

@cyrilletuzi

This comment has been minimized.

Copy link
Contributor Author

cyrilletuzi commented Dec 29, 2018

Integration test is failing but I don't think it's related to this PR.

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Dec 30, 2018

Thx, @cyrilletuzi!

Should the fix of the doc (about the bad default for version) be part of this PR or be another?

Let's make it a separate PR so we can merge it into the patch branch as well.

@gkalpak
Copy link
Member

gkalpak left a comment

Almost there 😃 A couple of minor comments.

Show resolved Hide resolved packages/service-worker/config/schema.json Outdated
Show resolved Hide resolved packages/service-worker/config/schema.json Outdated
Show resolved Hide resolved packages/service-worker/config/schema.json Outdated

@cyrilletuzi cyrilletuzi force-pushed the cyrilletuzi:swschema branch from 2310303 to e1e9469 Dec 30, 2018

@cyrilletuzi

This comment has been minimized.

Copy link
Contributor Author

cyrilletuzi commented Dec 30, 2018

Changes done. (Again a Travis fail unrelated to this PR)

@gkalpak

gkalpak approved these changes Jan 3, 2019

Copy link
Member

gkalpak left a comment

One minor comment. LGTM otherwise 🎉

BTW, it helps with reviews if you make subsequent changes in fixup commits (instead of amending the original commit)

Show resolved Hide resolved packages/service-worker/config/schema.json Outdated
@gkalpak
Copy link
Member

gkalpak left a comment

I just checked and it seems that schema.json is not published in dist/packages-dist/. (I had checked before and it was published, but maybe that was before moving it to service-worker/config/.)

Maybe package.json is special (in a way that schema.json is not) and we should include schema.json elsewhere. We need to pull in a bazel expert 😒

@cyrilletuzi

This comment has been minimized.

Copy link
Contributor Author

cyrilletuzi commented Mar 5, 2019

@gkalpak I took the chance to add a last change, following feeback from my experience: as the goal is to ensure there are no errors in ngsw-config.json, additionalProperties should be set to false for objects. Otherwise, I could do a typo and a wrong property would be allowed while the right one would be missing. As a consequence, $schema itself must be autothorized.

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Mar 5, 2019

Thx, @cyrilletuzi. I will take a look at the changes.

BTW, it would be better if we didn't work on the PR at the same time 😁
I am currently working on fixing CI; I'll let you know once I am done. But I think it is fine to merge this one and then do any enhancements/improvements in a follow-up PR.

@cyrilletuzi

This comment has been minimized.

Copy link
Contributor Author

cyrilletuzi commented Mar 5, 2019

@gkalpak Yep, sorry, I've seen you were working at the same time at the moment I pushed. :/

@gkalpak gkalpak force-pushed the cyrilletuzi:swschema branch from cc12814 to 4438b86 Mar 5, 2019

@cyrilletuzi cyrilletuzi requested review from angular/fw-compiler as code owners Mar 5, 2019

@gkalpak gkalpak force-pushed the cyrilletuzi:swschema branch from 4438b86 to 0613b38 Mar 5, 2019

Show resolved Hide resolved packages/service-worker/config/schema.json Outdated
@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Mar 5, 2019

I am done. CI is green. @cyrilletuzi, could you take care of #27859 (comment) (if needed)?
(Also happy to drop this commit from the PR, merge the PR and then open a new PR with the fix.)

@cyrilletuzi

This comment has been minimized.

Copy link
Contributor Author

cyrilletuzi commented Mar 5, 2019

@gkalpak You're totally right, it's fixed. I hope I didn't mess with your CI fixes, as I admit I'm quite lost with the git conflicts...

@gkalpak gkalpak force-pushed the cyrilletuzi:swschema branch from fd2cb1d to 4246ca1 Mar 5, 2019

@gkalpak gkalpak force-pushed the cyrilletuzi:swschema branch from 4246ca1 to df46ca0 Mar 5, 2019

@gkalpak

gkalpak approved these changes Mar 5, 2019

@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Mar 5, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "cla/google" is failing
    pending missing required labels: cla: yes
    pending forbidden labels detected: cla: no
    pending status "ci/angular: size" is pending
    pending status "ci/circleci: aio_preview" is pending
    pending status "ci/circleci: build-ivy-npm-packages" is pending
    pending status "ci/circleci: build-npm-packages" is pending
    pending status "ci/circleci: lint" is pending
    pending status "ci/circleci: test" is pending
    pending status "ci/circleci: test_aio" is pending
    pending status "ci/circleci: test_ivy_aot" is pending
    pending missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@AndrewKushnir AndrewKushnir added cla: yes and removed cla: no labels Mar 5, 2019

@googlebot

This comment has been minimized.

Copy link

googlebot commented Mar 5, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Mar 5, 2019

Thx, @cyrilletuzi! (No worries about the git merge/rebase mess. I cleaned it up 😉)
As soon as CI is happy, this should be good to go.

merge-asssitance: The fw-compiler, fw-forms and tools-language-service approvals are not needed (they were auto-requested by GitHub after a bad rebase).
merge-assistance: @alexeagle's approval should cover fw-integration. (It probably shows as pending because GitHub auto-re-requested after the aforementioned bad rebase.)

AndrewKushnir added a commit that referenced this pull request Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.