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.

Sign up for GitHub

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

Jump to bottom

📖 Getting started: Don't suggest NVM for Windows #22654

Merged
merged 4 commits into from
Jun 18, 2019
Merged

📖 Getting started: Don't suggest NVM for Windows #22654

merged 4 commits into from
Jun 18, 2019

Conversation

ChrisAntaki
Copy link
Contributor

  • Removes suggestion to use NVM for Windows, for a few reasons.
    • Just for some context, NVM for Windows "...is not the same thing as nvm, which is a completely separate project for Mac/Linux only." (source)
    • The suggested command (nvm install --lts) doesn't work with NVM for Windows, since the --lts flag isn't recognized.
    • There's an issue (VBScript engine issue coreybutler/nvm-windows#133) that's been open since January 2016 that prevented me from successfully using NVM for Windows on a work-issued laptop, even with temporary admin access.

@torch2424
Copy link
Contributor

@ChrisAntaki

These are all very good points, thank you for bringing this top our attention! 😄

cc @erwinmombay @rsimha @kristoferbaxter Since ya'll either work in infra, or have done AMP windows development in the past.

Thank you!

@torch2424
Copy link
Contributor

Also, @ChrisAntaki I noticed this PR also contains the changes from: #22647

Could you remove them here, that way we can continue these changes on the other PR?

Thank you!

@ChrisAntaki
Copy link
Contributor Author

Thanks for taking a look! I'll take care of those extra changes soon 😁

@ChrisAntaki
Copy link
Contributor Author

Extra changes have been removed 😄

@torch2424 torch2424 merged commit bbce96c into ampproject:master Jun 18, 2019
@torch2424
Copy link
Contributor

@ChrisAntaki Thanks for the contribution! 😄

@rsimha
Copy link
Contributor

rsimha commented Jun 18, 2019

LGTM. Thanks for the contribution!

@ChrisAntaki
Copy link
Contributor Author

Thanks guys!

@ChrisAntaki ChrisAntaki deleted the getting-started-remove-nvm-windows branch June 18, 2019 21:20
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
@ChrisAntaki @torch2424
📖 Getting started: Don't suggest NVM for Windows (ampproject#22654)
f0fbc14 
* Adds comment and renames script for better readability

* Updates name & comment

* Removes suggestion of NVM for Windows

* Removes eslint changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erwinmombay erwinmombay erwinmombay approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ChrisAntaki @torch2424 @rsimha @erwinmombay @googlebot