-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
ui: readme: update known-to-work yarn and node versions #25288
Conversation
Release note: None
Release note: None
package installation than NPM. NodeJS 6.x and Yarn 0.22.0 are known to work. | ||
[Chrome](https://www.google.com/chrome/), Google's internet browser. Unit tests | ||
are run using Chrome's "Headless" mode. | ||
package installation than NPM. NodeJS 8.4.x and Yarn 1.3.x are known to work. |
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.
hm, contributing.md
and package.json
still say node 6.x. Probably should bump in all of those places, or none. Not sure where our CI image is defined. Any opinion?
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.
build/builder/�Dockerfile
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 would drop version numbers in documentation. Keeping it at >=6
in package.json
is important, because that indicates support for the latest LTS version of Node.js, you don't want to drop that one.
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.
Unlike internal dep bumps, I don't think we want to push required versions on externally-installed build tools unless we've got a compelling reason for the change. Also we don't want to change the builder image until we bump the required version, otherwise there's a good chance we'll accidentally push the required version later without noticing it (consider the accidental upgrade to go 1.10 as an example).
There may be a reasonable argument for yarn
even if not for node
.
In any case, we'll want to send an alert to the mailing list to make sure people are warned ahead of a build change requiring a manual upgrade.
I’m fine leaving Node at 6, but I’m pretty sure Yarn <1.0 doesn’t work.
…On Mon, May 7, 2018 at 8:42 AM Andrew Couch ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/ui/README.md
<#25288 (comment)>
:
> @@ -16,9 +16,9 @@ you'll be able to access the UI at <http://localhost:8080>.
Our UI is compiled using a collection of tools that depends on
[Node.js](https://nodejs.org/) and are managed with
[Yarn](https://yarnpkg.com), a package manager that offers more deterministic
-package installation than NPM. NodeJS 6.x and Yarn 0.22.0 are known to work.
-[Chrome](https://www.google.com/chrome/), Google's internet browser. Unit tests
-are run using Chrome's "Headless" mode.
+package installation than NPM. NodeJS 8.4.x and Yarn 1.3.x are known to work.
Unlike internal dep bumps, I don't think we want to push required versions
on externally-installed build tools unless we've got a compelling reason
for the change. Also we don't want to change the builder image until we
bump the required version, otherwise there's a good chance we'll
accidentally push the required version later without noticing it (consider
the accidental upgrade to go 1.10 as an example).
There may be a reasonable argument for yarn even if not for node.
In any case, we'll want to send an alert to the mailing list to make sure
people are warned ahead of a build change requiring a manual upgrade.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#25288 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAcrQjB4zJjqmQH_D6DtjcRpoJe5Grrks5twGt-gaJpZM4TxkpA>
.
|
Superseded by #27262 |
No description provided.