-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Meta: Update ecmarkup version dependency #682
Conversation
@@ -8,6 +8,6 @@ env: | |||
language: node_js | |||
|
|||
node_js: | |||
- "5" | |||
- "6" |
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.
If you want to always point to latest, this can be "node"
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.
I prefer stable
.
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.
"stable" has no meaning in semver (and thus in node/io.js >= 1.0), where everything is stable.
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.
It is short for "latest stable", which does have meaning.
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.
Sure, but the latest not-stable is 0.11 - so it's mostly meaningless. Either way, nvm is going to remove "stable" at some point, so "node" is a safer long-term choice.
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.
while latest
is a nice terminology and maybe better than just "node", I agree stable
is ambigous, include LTS versions to the options I would wonder for it.
If there's a chance for breaking changes, link stable
to any stable release and do not update it anymore. It won't break anything and you'll have your deprecated alias.
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.
All of this is delightful discussion to have on the nvm repo when the time comes. For now, I'm suggesting "node" if we don't want to have to manually bump it to "7" etc in the future. If we don't like that, so be it.
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.
Back on topic, I believe testing against "stable"/"node"/LTS is an anti-pattern.
It means CI will unsolicitedly bump the major Node version whenever it becomes available without any previous test or knowledge from the authors. Major versions indicate breaking changes, and thus should be throughout tested (e.g. via a PR bumping the major version, like this one) in order to prevent breaking the build overnight.
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.
Fair counterpoint.
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.
@UltCombo I agree. We can have it be in the allowed failures so that, if a new version of node has incompatibilities, we will be notified but CI will not fail.
Looks good - I am fine with explicitly naming version 6 to test with. Note that ecmarkup 3.5 requires version 6 now. |
* upstream/master: (301 commits) Editorial: remove mistakenly committed change to eval-related super errors Normative: Point to the latest version of UTR15 (tc39#681) Meta: update ecmarkup version dependency (tc39#682) Editorial: Update a variable name in Annex B.3.3.1 to refer to the proper variable in FunctionDeclarationInstantiation's body, and add warning comments by all the spec algorithms monkey-patched by Annex B so that future refactoring doesn't create broken/dangling references in Annex B. (tc39#677) Meta: update ecmarkup version dependency Editorial: dfn-ify 'goal symbol' Editorial: TypeArray -> TypedArray (tc39#675) Editorial: Correct a wrong cross-reference in Annex C (tc39#674) Editorial: tweak whitespace and articles (tc39#667) Editorial: Tweak EnumerateObjectProperties informative definition (tc39#656) Editorial: Rename variable (indx --> index) (tc39#658) Editorial: Fix reference in ScriptEvaluation (tc39#659) Editorial: Consistent `undefined or null` order (tc39#660) Editorial: Refactor Array.prototype.toLocaleString (tc39#662) Editorial: Tweak Array.prototype.join (tc39#661) Simplify description of CreateImmutableBinding (tc39#654) Normative: Resolve template argument references (tc39#609) Editorial: Refactor Array.prototype.join (tc39#638) Editorial: contains --> Contains in ModuleListItem SS Normative: Allow initializers in ForInStatement heads (tc39#614) ...
Note that the Travis configuration has been updated accordingly.
Ref. tc39/ecmarkup#101.