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

More tree shaking #10355

Merged
merged 4 commits into from
Jun 8, 2022
Merged

More tree shaking #10355

merged 4 commits into from
Jun 8, 2022

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Jun 7, 2022

Re #10307

  • Mark moment as external since we are not using it (neither does jupyterlab/services)
  • Require rxjs sub modules only
  • 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 feature-requests.
  • 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).

@rebornix rebornix marked this pull request as ready for review June 8, 2022 00:28
@rebornix rebornix requested a review from a team as a code owner June 8, 2022 00:28
@@ -87,7 +87,7 @@ const config = {
}
]
},
externals: ['vscode', 'commonjs', 'electron'], // Don't bundle these
externals: ['vscode', 'commonjs', 'electron', 'moment'], // Don't bundle these
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to do something to prevent we don't end up in a situtation where tomorrow we pull in a 3rd party package or an existing 3rd party package ends up pulling in moment, then things will not work, or it will fail only in certian cases based on where and how the 3rd party package ends up using moment.

E.g. can we write up a linter to look at package-lock.json and ensure that nothing ever pulls in moment except for jupyter. Else I feel this is a risky change (as mentioned if some other package pulls it in, then things could break sometimes)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Contributor

@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.

Would like to ensure no other package pulls in moment, if they do, then we'd need to review that separately. Else we end up in a situation where something could break in the future.

@rebornix rebornix requested a review from DonJayamanne June 8, 2022 16:58
@rebornix
Copy link
Member Author

rebornix commented Jun 8, 2022

Thanks @DonJayamanne for the suggestion, we should validate the dependencies to see if any new package we use load moment and fail accidentally, great catch!

Added a gulp task to validate the lock file and added it to the pre-commit hook.

@@ -15,4 +15,5 @@ if [ -z "$changed" ]; then
fi

npm run generateTelemetry
npm run validateDependencies
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 maybe do this in the linter step for the build? I'm wary of adding yet more stuff to pre-commit. Pre-commit already takes like 60 seconds for me.

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 👍 I dislike slow commit, 60 seconds is too much. I'll move it to the linter since we always do PR.

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 maybe do this in the linter step for the build? I'm wary of adding yet more stuff to pre-commit. Pre-commit already takes like 60 seconds for me.

This is why I don't like the pre-commit hooks - slows down my workflow, fortunately the formatting pre-commit hook doesn't work for me.

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2022

Codecov Report

Merging #10355 (e1a999b) into main (98262a2) will decrease coverage by 0%.
The diff coverage is 100%.

❗ Current head e1a999b differs from pull request most recent head 3e5ad04. Consider uploading reports for the commit 3e5ad04 to get more accurate results

@@          Coverage Diff          @@
##           main   #10355   +/-   ##
=====================================
- Coverage    64%      64%   -1%     
=====================================
  Files       212      212           
  Lines      9557     9559    +2     
  Branches   1527     1530    +3     
=====================================
- Hits       6162     6151   -11     
- Misses     2924     2927    +3     
- Partials    471      481   +10     
Impacted Files Coverage Δ
src/platform/api/pythonApi.ts 69% <100%> (ø)
src/platform/common/cancellation.ts 62% <0%> (-6%) ⬇️
src/platform/common/platform/fileSystem.ts 81% <0%> (-5%) ⬇️
src/platform/errors/errorHandler.ts 63% <0%> (-3%) ⬇️
src/platform/common/utils.ts 70% <0%> (-2%) ⬇️
src/platform/api/kernelApi.ts 72% <0%> (-1%) ⬇️
src/platform/common/activeEditorContext.ts 93% <0%> (ø)
...rc/platform/common/dataScienceSurveyBanner.node.ts 70% <0%> (ø)
...rc/platform/common/extensionRecommendation.node.ts 81% <0%> (ø)
...tform/interpreter/display/visibilityFilter.node.ts 91% <0%> (ø)
... and 1 more

@rebornix rebornix merged commit cb7c2f3 into main Jun 8, 2022
@rebornix rebornix deleted the rebornix/tree-shaking-rxjs branch June 8, 2022 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants