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

added the regular expression for windows paths #143

Merged
merged 3 commits into from
Jan 3, 2019

Conversation

stukalin
Copy link
Contributor

Fixes #142.
Addresses #125.

The fix adds another include filter which contains a Windows path regular expression. I took the expression from here, replaced \ with / and added ember-service-worker and .js like in the glob pattern.

In theory this should not interfere with the glob expression which will work like it did before and should only cover the case described in #142.

@stukalin
Copy link
Contributor Author

stukalin commented Dec 6, 2018

@eshtadc would you mind to have a look at this one? We'd want the fix to be released (otherwise I end up having my dev repo npm-linked for ever)

// same as above but for windows paths like D:\my-ember\ember-service-worker\whatever\sw.js;
// the original one doesn't work when the source code and the %TMP% folder, which ember started to use after v3.5, are located on different logical drives
// which is quite common in Windows.
/^[a-z]:\/(?:[^\/:*?"<>|\r\n]+\/)*ember-service-worker\/(?:[^\/:*?"<>|\r\n]+\/)*[^\/:*?"<>|\r\n]*\.js$/i
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears that you are checking for "/" in the regex rather than "" in the comment. Which one is the intended? I seem to recall windows paths being more like your example but it's been a while....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is true. Windows paths do have \ as path separators, but somewhere in the code all the paths are "normalized" to use only /s. I may fix the comment though. What would you say?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the silence... holidays and all. I'm trying to find where that happens but I'm coming up short. I would just make the comment as explicit as it needs to be so that the next person looking at this understands why the regex is the way it is. Maybe same as above but for windows paths like D:\my-ember\ember-service-worker\whatever\sw.js; which are updated before this step to D:/my-ember/ember-service/worker/whatever/sw.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eshtadc done!

@eshtadc
Copy link
Contributor

eshtadc commented Jan 3, 2019

Thanks @stukalin! Release coming soon.

@eshtadc eshtadc merged commit 5ffad24 into DockYard:master Jan 3, 2019
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.

{{ROOT_URL}} not getting replaced during build on Windows when the code is on drive D:\
2 participants