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

Fix backslashes replacement. #774

Merged
merged 1 commit into from
Sep 5, 2017
Merged

Fix backslashes replacement. #774

merged 1 commit into from
Sep 5, 2017

Conversation

XhmikosR
Copy link
Contributor

This resulted in backslashes in the URL entries on Windows.

R: @jeffposnick @addyosmani @gauntface

Fixes #771

This resulted in backslashes in the URL entries on Windows.
@XhmikosR
Copy link
Contributor Author

The AppVeyor failure seems unrelated to this change.

On a side note, I could add a test if you could give me some hints, i.e. which file the test should be, if I should just check for backslashes in entry.urls, etc.

@gauntface
Copy link

Thanks for this @XkhmikosR.

I'm waiting on a Windows machine to test this a little more. I'm largely curious to see if I can add some tests that "fake" windows files paths so we can test on linux for Window "style" support.

@XhmikosR
Copy link
Contributor Author

@gauntface: you have AppVeyor so you can actually test Windows at any time https://www.appveyor.com/docs/how-to/rdp-to-build-worker/

It shouldn't be too hard to have a test for that. You could use https://nodejs.org/api/path.html#path_path_win32 or just hardcode the backslashes in a test case and just check if there are any in the output regardless of the OS.

@addyosmani
Copy link
Member

@gauntface Change here LGTM, but I don't have access to our AppVeyor account to follow the windows test instructions @XhmikosR linked up to. If you get a chance can you add me or test it out? Would love to see if we can unblock twbs/bootstrap#23533

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Sep 1, 2017

Would be nice if we had a test to ensure this doesn't happen again in the future, though :)

@gauntface
Copy link

Gonna accept this merge and dig into a way of testing this better. Windows has been awful for a while and frankly I'm stumped on a sane way to test for it.

@addyosmani tried adding you to AppVeyor but no idea how the hell it's "teams" works

@gauntface gauntface merged commit 40d210a into GoogleChrome:master Sep 5, 2017
@gauntface
Copy link

Thanks again for this @XhmikosR - this will be cut in the release tomorrow.

@XhmikosR XhmikosR deleted the patch-1 branch September 6, 2017 11:53
@MikeyBeLike
Copy link
Contributor

@gauntface thanks, can we get 2.0.1 published to npm too please

@jeffposnick
Copy link
Contributor

Sorry, there were a few hiccups with the 2.0.1 release, but https://www.npmjs.com/package/workbox-build (and workbox-cli/workbox-webpack-plugin) are published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants