-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fixing overlapping regex for #31 #32
Conversation
Overlapping regex matches were causing define:define and require:require strings to not be replaced correctly. Added a look ahead to fix this issue.
@tomwayson I was hoping to add a unit test here, but Ember builds out the app before testing. Any tips on running a unit test or two on the actual build hooks? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gbochenek!
I will pull and try this shortly.
Re: testing the addon code, I think you just write node tests. I don't know if @odoe did any for the regexes in ember-cli-amd (which I would assume would also be affected by this bug), or if @dmfenton or @mjuniper know of a more emberish way to test the code in index.js.
match: /([^A-Za-z0-9_#]|^|["])define(\W|["]|$)/g, | ||
replacement: '$1efineday$2' | ||
match: /([^A-Za-z0-9_#]|^|["])define(?=\W|["]|$)/g, | ||
replacement: '$1efineday' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the $2
from the replacements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the lookahead, there is no second grouping, and "$2" will be appended on every match.
@gbochenek nice work! I have pulled this and verified that it works in the dummy app as well as w/ I assume you able to verify that this fixed the production build issue that you saw in your app, right? If so, I'll merge and release. The regex tests would be nice to have, but I'd rather get this bug fixed sooner than later. I can open a separate issue to add node tests for index.js. |
Excellent. I've verified that this fixes the production build issue in my app as well. Lets ship it. |
Released in v0.1.4 |
Overlapping regex matches were causing define:define and require:require strings to not be replaced correctly. Added a look ahead to fix this issue.