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

Update dependencies #458

Merged
merged 8 commits into from
Mar 4, 2018
Merged

Update dependencies #458

merged 8 commits into from
Mar 4, 2018

Conversation

ahnpnl
Copy link
Collaborator

@ahnpnl ahnpnl commented Mar 3, 2018

  • Updated yarn.lock
  • Remove babel-core, jest-config because installing Jest already includes those 2. Since this repo only works with jest so it's mandatory to install Jest to be able to use this repo. Therefore we don't need to install specific those 2 dependencies

package.json Outdated
"babel-plugin-istanbul": "^4.1.4",
"babel-plugin-transform-es2015-modules-commonjs": "^6.24.1",
Copy link
Owner

Choose a reason for hiding this comment

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

We use these two directly in ts-jest so I'm not sure about this.

@GeeWee felt the need to include these as dependencies which makes me think this is needed

Copy link
Collaborator Author

@ahnpnl ahnpnl Mar 3, 2018

Choose a reason for hiding this comment

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

@kulshekhar I think you are right. I just tried to test it by installing locally the package on this branch and I couldn't see babel-plugin-transform-es2015-modules-commonjs. Only babel-core was installed by Jest. That is a bit strange.
I'll revert and bring that dependency back

Copy link
Owner

Choose a reason for hiding this comment

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

The rest of the changes look good to me so we can wait for @GeeWee to chime in on this

Copy link
Owner

Choose a reason for hiding this comment

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

I personally think that if we're importing and using something, that should explicitly be added as a dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm make sense 👍

@GeeWee
Copy link
Collaborator

GeeWee commented Mar 4, 2018

I'm a little hesitant to pin our version of babel to the one installed with e.g. jest. We support a few ranges of jest, and there's potential for breakage if their babel-core version varies with versions. I realize this is a pretty unfounded fear - I just have a bad feeling about having a transitive dependency on babel-core through jest - but the file size reductions would be nice.

@ahnpnl
Copy link
Collaborator Author

ahnpnl commented Mar 4, 2018

@GeeWee I think the size reductions is not so much. I reconsider between size reductions and the compatibility with different version of jest, I think the compatibility is more important than the size reductions in this case (similar to the customer oriented concept). If you and @kulshekhar agree to that, I can revert the changes of package.json.

@kulshekhar
Copy link
Owner

I can revert the changes of package.json

that sounds like a good idea to me

@GeeWee
Copy link
Collaborator

GeeWee commented Mar 4, 2018

Sounds good to me as well. I hope you'll keep contributing Ahn - we appreciate it!

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.

None yet

3 participants