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

Move esm to devDependencies #8586

Merged
merged 2 commits into from
Aug 13, 2019
Merged

Move esm to devDependencies #8586

merged 2 commits into from
Aug 13, 2019

Conversation

DatGreekChick
Copy link
Contributor

@DatGreekChick DatGreekChick commented Aug 2, 2019

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

@ryanhamley @mourner @asheemmamoowala

I moved esm to devDependencies and ran yarn install to update the yarn.lock file.

Using yarn removed any errors with tests that I was having previously in #8513 due to npm - thanks for that! I saw the docs, but didn't realize it would have this type of impact.

CLOSES #8450

Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

Thanks for taking another crack at this @DatGreekChick! The change itself looks fine, but there shouldn't be large changes to the yarn.lock file just from moving esm to devDependencies. In fact, if I try it myself, I don't get any changes at all to yarn.lock. Can you make sure your forked version of the repo is up to date and that you're using a newer version of Yarn? Older versions of Yarn have a tendency to unnecessarily change the lock file. For reference, I'm on Yarn 1.17.3 in Node 8.11.4

@asheemmamoowala asheemmamoowala added the skip changelog Used for PRs that do not need a changelog entry label Aug 2, 2019
@DatGreekChick
Copy link
Contributor Author

DatGreekChick commented Aug 2, 2019

Hey @ryanhamley! I updated my yarn version to 1.17.3 as well. I'm using node v11.6.0, though. I still see some changes to yarn.lock, but now the entire diff is 122 lines:

Elenis-MacBook-Pro : ~/mapbox-gl-js  (other/move-esm-to-dev-dependency)
🤓  git diff | wc -l
     122

I'll push my changes so they're visible here.

@DatGreekChick
Copy link
Contributor Author

Ok... that didn't actually work like I expected given my bash output 🤣
It's either because I'm using a higher version of node (I suspect this isn't it) or simply because when I run yarn install it's updating the minor versions if there are any new ones with packages using ^.

@asheemmamoowala
Copy link
Contributor

@DatGreekChick Could you rewrite your commit without the yarn.lock file?

@DatGreekChick
Copy link
Contributor Author

@asheemmamoowala will do 👍

@DatGreekChick
Copy link
Contributor Author

@asheemmamoowala I removed the yarn.lock file from the commit

@asheemmamoowala
Copy link
Contributor

@mapbox/gl-native Do you still depend on esm from the gl-js package for running integration tests? If not, we'd like to get this PR merged

@mourner
Copy link
Member

mourner commented Aug 9, 2019

@asheemmamoowala looks like Native has esm in their devDeps, so we're safe to move to devDeps as well.

@DatGreekChick
Copy link
Contributor Author

@ryanhamley can you dismiss your review?

@mourner mourner dismissed ryanhamley’s stale review August 13, 2019 07:27

yarn.lock removed from PR

@mourner mourner merged commit 0467fca into mapbox:master Aug 13, 2019
@DatGreekChick DatGreekChick deleted the other/move-esm-to-dev-dependency branch August 13, 2019 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changelog Used for PRs that do not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

esm dependency vunerability
4 participants