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 acorn to devDependencies #131

Merged
merged 1 commit into from
Feb 19, 2022
Merged

Conversation

ai
Copy link
Collaborator

@ai ai commented Feb 19, 2022

@LeaVerou I found another way to reduce npm package size.

dist/ files don’t use acorn. It was used only in notebook which is not part of npm package.

So we can move it from dependencies to devDependencies.

@@ -1,4733 +1,10578 @@
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

npm updated lock file according to .editorconfig. Should we keep it since new version of npm will do it for every contributor to the project?

@LeaVerou
Copy link
Member

Oh I thought devDependencies were for deps related to the build process, whereas dependencies were for deps actually used by the project. No?

Not sure I understand your followup comment. Keep what?

@ai
Copy link
Collaborator Author

ai commented Feb 19, 2022

Oh I thought devDependencies were for deps related to the build process, whereas dependencies were for deps actually used by the project.

The problem is that we are using the same repo for website and for npm package. So we technically have two sets of dependencies: dependencies/devDependencies for npm package and dependencies/devDependencies for website.

There is no simple solution for this problem. I suggest to keep dependencies for npm package dependencies and devDependencies for website/build tools.

Not sure I understand your followup comment. Keep what?

When I called npm install it updated package-lock.json from old version 1 to new version 2. I have npm 8, which have new lockfile format.

Copy link
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

I guess moving acorn to devDependencies is fine, can't think of how it may affect anything negatively, so I'm gonna go ahead and approve this.
Then you can merge it whenever you're happy with package-lock.json? Not sure whether you want to keep it or remove it from this PR. Up to you!

@ai
Copy link
Collaborator Author

ai commented Feb 19, 2022

Then you can merge it whenever you're happy with package-lock.json? Not sure whether you want to keep it or remove it from this PR. Up to you!

Let’s keep it. It will make contribution simpler for new developer (who will have npm >7 too).

@LeaVerou
Copy link
Member

Sure. Are you able to merge it yourself with the approved review or should I merge it?

@ai
Copy link
Collaborator Author

ai commented Feb 19, 2022

Nope, I do not have Merge button

@LeaVerou
Copy link
Member

LeaVerou commented Feb 19, 2022

I can add you as a collaborator if you plan to work more on the project, what do you think?

@ai
Copy link
Collaborator Author

ai commented Feb 19, 2022

I can add you as a collaborator if you plan to work more on the project, what do you think?

I am not sure about my contribution in month later.

Let’s keep PR flow for now.

@LeaVerou LeaVerou merged commit 9687c83 into color-js:master Feb 19, 2022
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.

2 participants