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.

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

fix: update envinfo + implementation, update issue_template #4375

Merged
merged 2 commits into from
Apr 30, 2018

Conversation

tabrindle
Copy link
Contributor

@tabrindle tabrindle commented Apr 27, 2018

This PR fixes a few outstanding issues, and supersedes #3859 and #3797.

There were a couple issues with envinfo, and the issue template that never got resolved - this is my attempt to fix all of that. Also, envinfo has been rewritten for size and speed since originally integrated, and a few more features have been added. The command is the same create-react-app --info, but now returns more info, like this:

  System:
    OS: macOS High Sierra 10.13
    CPU: x64 Intel(R) Core(TM) i7-4870HQ CPU @ 2.50GHz
  Binaries:
    Node: 8.11.0 - ~/.nvm/versions/node/v8.11.0/bin/node
    Yarn: 1.5.1 - ~/.yarn/bin/yarn
    npm: 5.6.0 - ~/.nvm/versions/node/v8.11.0/bin/npm
  Browsers:
    Chrome: 65.0.3325.181
    Firefox: 59.0.2
    Safari: 11.0
  npmPackages:
    react: ^16.3.2 => 16.3.2 
    react-dom: ^16.3.2 => 16.3.2 
    react-scripts: 1.1.4 => 1.1.4 
  npmGlobalPackages:
    create-react-app: 1.5.2

screen shot 2018-04-27 at 3 11 03 pm

Since you no longer recommend installing CRA (using via npx instead), also adding npx envinfo --preset create-react-app might be a faster option. envinfo is extremely light and fast via npx, and can be locked to a specific version - npx envinfo@5.2.0 --preset create-react-app, or just installed directly yarn global add envinfo && envinfo --preset create-react-app in the case of pre npm 5.2 installs. I can add this to the template upon request.

@bondz This may interest you.

@iansu
Copy link
Contributor

iansu commented Apr 27, 2018

This looks great. Thanks!

@tabrindle
Copy link
Contributor Author

I'm really not sure what's going on with ci - the only dep change should be envinfo, which doesn't use babel-loader, anywhere in the dependency tree.

npx envinfo --npmPackages babel-loader --fullTree --showNotFound

  npmPackages:
    babel-loader: Not Found

@iansu
Copy link
Contributor

iansu commented Apr 27, 2018

We've been having issues with node 6 tests on Travis recently but this looks like something else. And the same failure is happening on Appveyor. I'll re-trigger the tests on Travis and see what happens.

@iansu
Copy link
Contributor

iansu commented Apr 28, 2018

I have no idea why the tests keep failing. I wonder if it's because you're targeting master instead of next? Everything is going into next right now and I think this should too. We're not planning to release any new 1.x versions unless we absolutely have to. Can you update your PR and change the base branch to next?

# Conflicts:
#	packages/create-react-app/package.json
@tabrindle tabrindle changed the base branch from master to next April 28, 2018 01:20
@tabrindle
Copy link
Contributor Author

ok - rebased and changed target branch - maybe this will fix?

2. `npm -v`:
3. `yarn --version` (if you use Yarn):
4. `npm ls react-scripts` (if you haven’t ejected):
Run the following command in your react app's folder in terminal.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think React should be capitalized.

{
System: ['OS', 'CPU'],
Binaries: ['Node', 'npm', 'Yarn'],
Browsers: ['Chrome', 'Firefox', 'Safari'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add Edge and IE here to support Windows users?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am 100,000% not a windows user - I did my best, and as of envinfo@5.4.0, they should both be there. If someone could test this out npx envinfo@5.4.0 --browsers

This shows up for me on a vagrant box.

  Browsers:
    Edge: 20.10240.17146.0
    Internet Explorer: 11.0.10240.17443

Will update this PR too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works on Windows 10. And outputs

Browsers:
    Edge: 41.16299.371.0
    Internet Explorer: 11.0.16299.371

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome thanks for testing - Ill update the PR to the latest version now.

@iansu
Copy link
Contributor

iansu commented Apr 28, 2018

Well that sure didn't work. We've lost Travis entirely and Appveyor thinks the changes are unmergeable. 😞

I'd like to avoid having you create a new PR but I'm not really sure what else to do at this point. @Timer @gaearon any suggestions?

@tabrindle
Copy link
Contributor Author

tabrindle commented Apr 28, 2018 via email

@@ -24,7 +24,7 @@
"chalk": "^1.1.3",
"commander": "^2.9.0",
"cross-spawn": "^4.0.0",
"envinfo": "3.4.2",
"envinfo": "5.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 5.4.0, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup 👍

@Timer
Copy link
Contributor

Timer commented Apr 30, 2018

I think we've added too many tests and Node 6 is too slow in comparison with Node 8; we can probably drop it from the test matrix now considering it is in maintenance as of today (and add Node 10).

@iansu iansu added this to the 2.0.0 milestone Apr 30, 2018
@iansu iansu merged commit 7b2eae1 into facebook:next Apr 30, 2018
@iansu
Copy link
Contributor

iansu commented Apr 30, 2018

Thanks!

@tabrindle
Copy link
Contributor Author

Thanks to @bondz for his prior PR and review 🎉
Also, thanks @iansu for your help and review 👍

kellyrmilligan added a commit to kellyrmilligan/create-react-app that referenced this pull request May 2, 2018
* upstream/next: (35 commits)
  Update envinfo and issue template (facebook#4375)
  Update sass-loader to 7.0.1 (facebook#4376)
  Support package distribution tags (facebook#4350)
  fix broken css module support in prod (facebook#4361)
  Bumped jest version to 22.4.1 (facebook#4362)
  bump babel 7 to beta 46
  bump lint-staged to node 10 compatible version
  documentation: Added License to the README.md (facebook#4294)
  Bump `fsevents`. (facebook#4331)
  Fix typo in e2e-simple.sh comment (facebook#4323)
  Add Sass loader (facebook#4195)
  Fix some typos in README.md (facebook#4286)
  Added learnstorybook.com to Storybook links (facebook#4298)
  Document multiple build environments via `env-cmd` facebook#4071 (facebook#4117)
  Fixed link to CSS imports blog post
  Update CSS Modules localIndetName (facebook#4192)
  Enable loose mode for `class-properties` (facebook#4248)
  bump babel 7 beta (facebook#4253)
  Small typo fix facebook#4217
  Changelog for 1.1.4
  ...
kellyrmilligan added a commit to kellyrmilligan/create-react-app that referenced this pull request May 2, 2018
* next: (35 commits)
  Update envinfo and issue template (facebook#4375)
  Update sass-loader to 7.0.1 (facebook#4376)
  Support package distribution tags (facebook#4350)
  fix broken css module support in prod (facebook#4361)
  Bumped jest version to 22.4.1 (facebook#4362)
  bump babel 7 to beta 46
  bump lint-staged to node 10 compatible version
  documentation: Added License to the README.md (facebook#4294)
  Bump `fsevents`. (facebook#4331)
  Fix typo in e2e-simple.sh comment (facebook#4323)
  Add Sass loader (facebook#4195)
  Fix some typos in README.md (facebook#4286)
  Added learnstorybook.com to Storybook links (facebook#4298)
  Document multiple build environments via `env-cmd` facebook#4071 (facebook#4117)
  Fixed link to CSS imports blog post
  Update CSS Modules localIndetName (facebook#4192)
  Enable loose mode for `class-properties` (facebook#4248)
  bump babel 7 beta (facebook#4253)
  Small typo fix facebook#4217
  Changelog for 1.1.4
  ...
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants