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

fix(service-worker): use posix path resolution for generation of ngsw… #19527

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@Delagen
Contributor

Delagen commented Oct 3, 2017

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] 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: #19521

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Oct 3, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot commented Oct 3, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@googlebot googlebot added the cla: no label Oct 3, 2017

@Delagen

This comment has been minimized.

Show comment
Hide comment
@Delagen

Delagen Oct 3, 2017

Contributor
Contributor

Delagen commented Oct 3, 2017

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Oct 3, 2017

CLAs look good, thanks!

googlebot commented Oct 3, 2017

CLAs look good, thanks!

@ocombe

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Oct 10, 2017

Contributor

@Delagen hello, can you run gulp format on your code to fix the linting error? (check that everything is ok with gulp lint)

Contributor

ocombe commented Oct 10, 2017

@Delagen hello, can you run gulp format on your code to fix the linting error? (check that everything is ok with gulp lint)

@ocombe

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Oct 10, 2017

Contributor

Thanks, can you squash your commits as well ?

Contributor

ocombe commented Oct 10, 2017

Thanks, can you squash your commits as well ?

@Delagen

This comment has been minimized.

Show comment
Hide comment
@Delagen

Delagen Oct 10, 2017

Contributor

@ocombe OK

Contributor

Delagen commented Oct 10, 2017

@ocombe OK

@ocombe

ocombe approved these changes Oct 10, 2017

Tested on windows and it fixes the issue

@ocombe

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Oct 10, 2017

Contributor

Did you notice that it also rewrites the urls patterns the same way on windows? Do you think that you could fix this in the same PR as well?

For example with the following config file:

{
  "index": "/index.html",
  "appData": {
    "test": true
  },
  "assetGroups": [{
    "name": "appshell",
    "resources": {
      "files": [
        "/assets/**/*"
      ],
      "versionedFiles": [
        "/**/*.html",
        "/**/*.js",
        "/**/*.css",
        "!/ngsw-worker.js"
      ],
      "urls": [
        "/test"
      ]
    }
  }],
  "dataGroups": [{
    "name": "api-freshness",
    "urls": [
      "/timeline"
    ],
    "cacheConfig": {
      "maxSize": 100,
      "maxAge": "3d",
      "timeout": "1m",
      "strategy": "freshness"
    }
  }]
}

the urls (in this case /test and /timeline) will be outputed \\/test and \\/timeline in the ngsw.json file:

{
  "configVersion": 1,
  "index": "/index.html",
  "appData": {
    "test": true
  },
  "assetGroups": [
    {
      "name": "appshell",
      "installMode": "prefetch",
      "updateMode": "prefetch",
      "urls": [
        "/index.html",
        "/inline.bundle.js",
        "/main.bundle.js",
        "/polyfills.bundle.js",
        "/styles.bundle.css",
        "/vendor.bundle.js"
      ],
      "patterns": [
        "\\/test"
      ]
    }
  ],
  "dataGroups": [
    {
      "name": "api-freshness",
      "patterns": [
        "\\/timeline"
      ],
      "strategy": "freshness",
      "maxSize": 100,
      "maxAge": 259200000,
      "timeoutMs": 60000,
      "version": 1
    }
  ],
  "hashTable": {
    ...
  }
}
Contributor

ocombe commented Oct 10, 2017

Did you notice that it also rewrites the urls patterns the same way on windows? Do you think that you could fix this in the same PR as well?

For example with the following config file:

{
  "index": "/index.html",
  "appData": {
    "test": true
  },
  "assetGroups": [{
    "name": "appshell",
    "resources": {
      "files": [
        "/assets/**/*"
      ],
      "versionedFiles": [
        "/**/*.html",
        "/**/*.js",
        "/**/*.css",
        "!/ngsw-worker.js"
      ],
      "urls": [
        "/test"
      ]
    }
  }],
  "dataGroups": [{
    "name": "api-freshness",
    "urls": [
      "/timeline"
    ],
    "cacheConfig": {
      "maxSize": 100,
      "maxAge": "3d",
      "timeout": "1m",
      "strategy": "freshness"
    }
  }]
}

the urls (in this case /test and /timeline) will be outputed \\/test and \\/timeline in the ngsw.json file:

{
  "configVersion": 1,
  "index": "/index.html",
  "appData": {
    "test": true
  },
  "assetGroups": [
    {
      "name": "appshell",
      "installMode": "prefetch",
      "updateMode": "prefetch",
      "urls": [
        "/index.html",
        "/inline.bundle.js",
        "/main.bundle.js",
        "/polyfills.bundle.js",
        "/styles.bundle.css",
        "/vendor.bundle.js"
      ],
      "patterns": [
        "\\/test"
      ]
    }
  ],
  "dataGroups": [
    {
      "name": "api-freshness",
      "patterns": [
        "\\/timeline"
      ],
      "strategy": "freshness",
      "maxSize": 100,
      "maxAge": 259200000,
      "timeoutMs": 60000,
      "version": 1
    }
  ],
  "hashTable": {
    ...
  }
}
@ocombe

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Oct 10, 2017

Contributor

Actually I haven't checked on linux, maybe it does the same thing for patterns

Contributor

ocombe commented Oct 10, 2017

Actually I haven't checked on linux, maybe it does the same thing for patterns

@Delagen

This comment has been minimized.

Show comment
Hide comment
@Delagen

Delagen Oct 10, 2017

Contributor

I'll check it. I think on linux all ok. I din't check urls, as it missing from my config

Contributor

Delagen commented Oct 10, 2017

I'll check it. I think on linux all ok. I din't check urls, as it missing from my config

@Delagen

This comment has been minimized.

Show comment
Hide comment
@Delagen

Delagen Oct 10, 2017

Contributor

@ocombe I think It's expected behavior due globToRegex code. It escape slash with backslash

Contributor

Delagen commented Oct 10, 2017

@ocombe I think It's expected behavior due globToRegex code. It escape slash with backslash

@ocombe

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Oct 10, 2017

Contributor

Ok good then :) thanks for checking it out

Contributor

ocombe commented Oct 10, 2017

Ok good then :) thanks for checking it out

@Delagen

This comment has been minimized.

Show comment
Hide comment

@ocombe ocombe requested a review from alxhub Oct 16, 2017

@alxhub

alxhub approved these changes Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment