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

Don't use npx for webpack build and update npm version #9387

Merged
merged 9 commits into from
Jan 7, 2020

Conversation

IanMatthewHuff
Copy link
Member

For #7197

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Appropriate comments and documentation strings in the code
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • The wiki is updated with any design decisions/details.

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Will check, didn't know we could use npm run webpack
Hmmm

@IanMatthewHuff
Copy link
Member Author

Context:
npx broke on windows with the -n parameter that we use at version 6.11.
npm/npx#5
As far as I can see npx doesn't seem to be getting much update attention, so I looked if I could move away from it.
npm/npx#30

The main missing thing from npm run was setting the memory limit up so I used the cross-env method from here:
npm/npm#12238

@IanMatthewHuff
Copy link
Member Author

IanMatthewHuff commented Jan 4, 2020

Will check, didn't know we could use npm run webpack
Hmmm

Worked for my local vscode build, npm run commands, and npm run package on windows and my mac. Something is up on the build machine though. I'll check that out.

@codecov-io
Copy link

codecov-io commented Jan 4, 2020

Codecov Report

Merging #9387 into master will increase coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9387      +/-   ##
==========================================
+ Coverage   60.73%   60.97%   +0.24%     
==========================================
  Files         550      529      -21     
  Lines       28748    28552     -196     
  Branches     4348     4333      -15     
==========================================
- Hits        17459    17409      -50     
+ Misses      10338    10192     -146     
  Partials      951      951
Impacted Files Coverage Δ
src/client/testing/serviceRegistry.ts 49.09% <0%> (-47.28%) ⬇️
src/client/testing/codeLenses/main.ts 40% <0%> (-30%) ⬇️
src/client/datascience/jupyter/jupyterUtils.ts 61.11% <0%> (-27.78%) ⬇️
src/client/formatters/serviceRegistry.ts 75% <0%> (-25%) ⬇️
...t/datascience/jupyter/jupyterDataRateLimitError.ts 50% <0%> (-25%) ⬇️
...ience/jupyter/jupyterDebuggerRemoteNotSupported.ts 60% <0%> (-20%) ⬇️
...cience/jupyter/jupyterDebuggerNotInstalledError.ts 50% <0%> (-16.67%) ⬇️
src/client/common/utils/icons.ts 83.33% <0%> (-16.67%) ⬇️
src/client/api.ts 78.57% <0%> (-14.29%) ⬇️
src/client/datascience/cellFactory.ts 59.52% <0%> (-11.91%) ⬇️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec8d808...c1ffd0a. Read the comment docs.

@IanMatthewHuff
Copy link
Member Author

Sorry still a draft at this point. Forgot to mark as such.

@@ -2928,7 +2930,7 @@
"nock": "^10.0.6",
"node-has-native-dependencies": "^1.0.2",
"node-html-parser": "^1.1.13",
"nyc": "^14.1.1",
"nyc": "^15.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

NYC just flopped over with the newer NPM on windows. Was thankfully just fixed in a recent update to nyc 15.

@IanMatthewHuff
Copy link
Member Author

Currently running nightly against this PR as well.

@@ -2699,11 +2699,11 @@
"scripts": {
"package": "gulp clean && gulp prePublishBundle && vsce package -o ms-python-insiders.vsix",
"compile": "tsc -watch -p ./",
"compile-webviews-watch": "npx npx -n --max_old_space_size=9096 webpack --config ./build/webpack/webpack.datascience-ui.config.js --watch",
"compile-webviews-watch": "cross-env NODE_OPTIONS=--max_old_space_size=9096 webpack --config ./build/webpack/webpack.datascience-ui.config.js --watch",
Copy link

Choose a reason for hiding this comment

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

cross [](start = 35, length = 5)

Is this a replacement for npx?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rchiodo No, basically just calling webpack and having webpack as a script entry replaces NPX. But cross-env is needed because of that -n parameter that we used with npx. That parameter gets passed as a node option to npm run. So to get around not having that cross-env is just a cross platform way to set an environment variable. That env-var then picks up the old space size setting when running webpack.

Copy link

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link

@kimadeline kimadeline left a comment

Choose a reason for hiding this comment

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

🚢

@IanMatthewHuff IanMatthewHuff merged commit b765037 into microsoft:master Jan 7, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 14, 2020
@IanMatthewHuff IanMatthewHuff deleted the dev/ianhu/updateNPM branch July 9, 2020 22:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants