chore(build): fix version placeholder matching by gkalpak · Pull Request #15016 · angular/angular.js

gkalpak · 2016-08-11T23:34:50Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.

What is the current behavior? (You can also link to an open issue here)

angular.version === {
  ...
  major: 'NG_VERSION_MAJOR',
  minor: 'NG_VERSION_MINOR',
  dot: 'NG_VERSION_DOT',
  ...
}

What is the new behavior (if this is a feature change)?

angular.version === {
  ...
  major: 1,
  minor: 5,
  dot: 9,
  ...
}

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
~~Docs have been added / updated (for bug fixes / features)~~

Other information:
During the build task, the version placeholders will be replaced with the actual values using a
RegExp, which expects the placeholders to be surrounded by double quotes. By replacing the quotes
from double to single in #15011, the RegExp was not able to match the placeholders.

Btw, this has broken ngMaterial (since they polyfill $animateCss based on the value of angular.version.minor).

gkalpak · 2016-08-12T08:15:48Z

For reference, here are the RegExps that match and replace the version placeholders: lib/grunt/utils.js#L125-L130

Narretz · 2016-08-12T08:19:03Z

test/e2e/tests/version.spec.js

+    expect(version.then(validate)).toBe(true);
+
+    function validate(ver) {
+      return ver.full.indexOf([ver.major, ver.minor, ver.dot].join('.')) === 0;


Why not just ver.full === [ver.major, ver.minor, ver.dot].join('.')?

That is not always true. E.g. full can be 1.5.0-rc.2 or 1.5.9-build.4949 or 1.5.9-local+sha.859348c (this last one is what we actually have in the tests most of the time).

Makes sense. Can you add a comment with that info?

During the `build` task, the version placeholders will be replaced with the actual values using a RegExp, which expects the placeholders to be surrounded by double quotes. By replacing the quotes from double to single in angular#15011, the RegExp was not able to match the placeholders. (For reference, the RegExps that match and replace the version placeholders are in [lib/grunt/utils.js][1].) [1]: https://github.com/angular/angular.js/blob/859348c7f61ff5f93b9f81eb7f46842bd018d8e3/lib/grunt/utils.js#L125-L130

mgol · 2016-08-12T13:21:11Z

How about we don't change what you changed here and keep our style but update the regexes so that they accept both string delimiter types? E.g. we'd change:

.replace(/"NG_VERSION_FULL"/g, NG_VERSION.full)

to:

.replace(/(?:'|")NG_VERSION_FULL(?:'|")/g, NG_VERSION.full)

etc.?

EDIT: or:

.replace(/(\\?(?:'|"))NG_VERSION_FULL\1/g, NG_VERSION.full)

gkalpak · 2016-08-12T14:53:55Z

lib/grunt/utils.js

+      .replace(/(['"])NG_VERSION_MAJOR\1/, NG_VERSION.major)
+      .replace(/(['"])NG_VERSION_MINOR\1/, NG_VERSION.minor)
+      .replace(/(['"])NG_VERSION_DOT\1/, NG_VERSION.patch)
+      .replace(/(['"])NG_VERSION_CDN\1/, NG_VERSION.cdn)


I could find this anywhere inside the codebase. It is probably not needed, but left it just in case.

gkalpak · 2016-08-12T14:56:14Z

I changed the RegExps to accept both types of quotes. @mgol, PTAL.
(I didn't account for escaped quotes, since we don't need them anywhere if we can use ' and " interchangeably.)

mgol · 2016-08-12T15:02:11Z

LGTM

During the `build` task, the version placeholders will be replaced with the actual values using a RegExp, which expects the placeholders to be surrounded by double quotes. By replacing the quotes from double to single in #15011, the RegExp was not able to match the placeholders. (For reference, the RegExps that match and replace the version placeholders are in [lib/grunt/utils.js][1].) [1]: https://github.com/angular/angular.js/blob/859348c7f61ff5f93b9f81eb7f46842bd018d8e3/lib/grunt/utils.js#L125-L130 Closes #15016

During the `build` task, the version placeholders will be replaced with the actual values using a RegExp, which expects the placeholders to be surrounded by double quotes. By replacing the quotes from double to single in angular#15011, the RegExp was not able to match the placeholders. (For reference, the RegExps that match and replace the version placeholders are in [lib/grunt/utils.js][1].) [1]: https://github.com/angular/angular.js/blob/859348c7f61ff5f93b9f81eb7f46842bd018d8e3/lib/grunt/utils.js#L125-L130 Closes angular#15016

During the `build` task, the version placeholders will be replaced with the actual values using a RegExp, which expects the placeholders to be surrounded by double quotes. By replacing the quotes from double to single in #15011, the RegExp was not able to match the placeholders. (For reference, the RegExps that match and replace the version placeholders are in [lib/grunt/utils.js][1].) [1]: https://github.com/angular/angular.js/blob/859348c7f61ff5f93b9f81eb7f46842bd018d8e3/lib/grunt/utils.js#L125-L130 Closes #15016

googlebot added the cla: yes label Aug 11, 2016

gkalpak added type: chore type: bug frequency: high needs: review severity: regression component: misc core labels Aug 11, 2016

gkalpak added this to the 1.5.9 milestone Aug 11, 2016

Narretz reviewed Aug 12, 2016
View reviewed changes

gkalpak force-pushed the chore-build-fix-version-placeholders branch from 30ba653 to 337b9a9 Compare August 12, 2016 08:21

gkalpak added 2 commits August 12, 2016 11:45

fixup! chore(build): fix version placeholder matching

6883eb4

fixup! chore(build): fix version placeholder matching

07e6958

gkalpak force-pushed the chore-build-fix-version-placeholders branch from df32fc6 to 07e6958 Compare August 12, 2016 09:25

fixup! chore(build): fix version placeholder matching

c3adae9

gkalpak reviewed Aug 12, 2016
View reviewed changes

gkalpak closed this in 6304cde Aug 12, 2016

gkalpak deleted the chore-build-fix-version-placeholders branch August 12, 2016 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(build): fix version placeholder matching#15016

chore(build): fix version placeholder matching#15016
gkalpak wants to merge 4 commits intoangular:masterfrom
gkalpak:chore-build-fix-version-placeholders

gkalpak commented Aug 11, 2016

Uh oh!

gkalpak commented Aug 12, 2016

Uh oh!

Narretz Aug 12, 2016

Uh oh!

gkalpak Aug 12, 2016 •

edited

Loading

Uh oh!

Narretz Aug 12, 2016

Uh oh!

mgol commented Aug 12, 2016 •

edited

Loading

Uh oh!

gkalpak Aug 12, 2016

Uh oh!

gkalpak commented Aug 12, 2016

Uh oh!

mgol commented Aug 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

gkalpak commented Aug 11, 2016

Uh oh!

gkalpak commented Aug 12, 2016

Uh oh!

Narretz Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

mgol commented Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Aug 12, 2016

Uh oh!

mgol commented Aug 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gkalpak Aug 12, 2016 •

edited

Loading

mgol commented Aug 12, 2016 •

edited

Loading