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: make sure devDependencies are marked as such #110

Merged
merged 1 commit into from
Dec 31, 2019

Conversation

beeman
Copy link
Contributor

@beeman beeman commented Dec 28, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

This should be a start to tackle this issue:

Issue Number: #88

What is the new behavior?

Dependencies needed only during development are moved to the devDependencies object.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I checked these pages and found that almost none of the devDependencies are marked as such:

https://www.npmjs.com/package/@scullyio/scully?activeTab=dependencies
https://www.npmjs.com/package/@scullyio/init?activeTab=dependencies

There might be more deps that can be moved, but this should be a good start.

@SanderElias
Copy link
Contributor

@beeman, we need to talk this one through. As Scully is a dev-time tool, arent all of them devDepedencies?
Also ,the top-level package.json is actually not being distributed to anywhere, but just four our own development mono-repo. That means that everything in there is a devDepdency right?

@beeman
Copy link
Contributor Author

beeman commented Dec 30, 2019

@SanderElias scully being a dev-time tool is not relevant here.

We should only list the dependencies that are needed in runtime in the dependencies object, the rest (what's needed to build scully itself) goes into devDependencies. This keeps the fotoprint small and prevents downloading and installing unneeded deps.

With regards to the top-level package, as it's not distributed it doesn't really matter but you are right, they could all be devDependencies.

@aaronfrost
Copy link
Contributor

This LGTM, but deferring to @SanderElias for approval.

@SanderElias
Copy link
Contributor

We should only list the dependencies that are needed in runtime in the dependencies object, the rest (what's needed to build scully itself) goes into devDependencies. This keeps the fotoprint small and prevents downloading and installing unneeded deps.

You are right!

@SanderElias SanderElias self-requested a review December 31, 2019 10:57
Copy link
Contributor

@SanderElias SanderElias left a comment

Choose a reason for hiding this comment

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

LGTM

@SanderElias SanderElias merged commit 78dbdf2 into scullyio:master Dec 31, 2019
@beeman beeman deleted the beeman/scully-dev-deps branch December 31, 2019 15: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.

3 participants