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.

Sign up for GitHub

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

Jump to bottom

Delete package-lock.json #1216

Closed
wants to merge 4 commits into from
Closed

Delete package-lock.json #1216

wants to merge 4 commits into from

Conversation

trusktr
Copy link
Member

@trusktr trusktr commented Jun 13, 2020 •
edited

I think libraries should not include package-lock.json.

  • The npm command does not publish them to npmjs.com, so end users don't get benefit from them.
  • We shouldn't attempt to lock dependencies using shrinkwrap anyways (increases the chance of duplicate dependencies in end user projects, which can cause issues, I can provide some links if curious).
  • The main reason to remove package-lock.json is because as we authors develop Docsify, we can catch in-range breaking changes to any dependencies and fix them before users receive the errors. Having package-lock.json otherwise prevents us from catching those bugs sooner.
  • We can manually pin dependencies as a last resort in package.json if we really need to.

Wdyt? Any reason to keep it?

@vercel
Copy link

vercel bot commented Jun 13, 2020 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/44415l3p5
✅ Preview: https://docsify-preview-git-delete-package-lock.docsify-core.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 13, 2020 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9842b80:

Sandbox Source
wonderful-khayyam-opwkm Configuration

@trusktr trusktr mentioned this pull request Jun 13, 2020
Merged
19 tasks
anikethsaha
anikethsaha requested changes Jun 13, 2020
View reviewed changes
Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

If we are removing this, I guess we should remove the lockfile for packages/docsify-server-renderer as well and put them in .gitignore

@trusktr
Copy link
Member Author

trusktr commented Jun 13, 2020 •
edited

If we are removing this, I guess we should remove the lockfile for packages/docsify-server-renderer as well and put them in .gitignore

Good catch! Thanks for reminding me! Good to have multiple 👀 . :)

@trusktr
- also ignore package-lock in the server-render package
3dd37ea 
- ignore package-lock in both .npmrc and .gitignore
@trusktr
Copy link
Member Author

trusktr commented Jun 13, 2020

There we go. I added the config in .npmrc which tells npm not to generate package-lock.json files, but some tools (like Lerna) don't obey this option so in any other cases beside using npm the .gitignore addition will help.

@trusktr trusktr requested a review from anikethsaha June 13, 2020 20:34
@trusktr
Copy link
Member Author

trusktr commented Jun 13, 2020

Let me fix the ironic conflict with package-lock.json. :)

@trusktr
Merge branch 'develop' into delete-package-lock
170bc87 
* develop:
  Update build/css.js
  use a port for the tests that doesn't collide with common local server ports
  Revert "ensure that the test script runs a prod build"
  update outdated comment
  ensure that the test script runs a prod build
  simplify import
  remove some unused code and accept eslint changes
  remove the DOCSIFY global made by Rollup, and move Docsify into a separate file where we can import it from tests while leaving the entry point for the bundle without any exports so that Rollup will not create a global variable from it
  feat: update src/core/index.js to export all global APIs, deprecate old globals in favor of a single global DOCSIFY, and add tests for this
  add build error handling so builds don't silently fail
  update docs regarding configs as functions
  add editorconfig (tells editors which basic text format to use)
  add tests to make sure the global and plugin APIs are available and that configs can be functions
  allow the user's $docsify config to be a function that receives as input the Docsify instance
jhildenbiddle
jhildenbiddle previously approved these changes Jun 13, 2020
View reviewed changes
Copy link
Member

@jhildenbiddle jhildenbiddle left a comment •
edited

Choose a reason for hiding this comment

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

Conceptually this seems okay to me. That said, I'm not familiar with some of the tools used in this repo (like Lerna) so I can't comment on if/how this might affect builds in different environments.

Looks like there are a few tests failing which will need to be addressed though.

@anikethsaha
Copy link
Member

I think there is some problem with CI related to monorepo (lerna).
Does lerna requires the lock file in order to have the same env ?

@trusktr
Copy link
Member Author

trusktr commented Jun 15, 2020

Ah, the tests fail because Lerna is running in ci mode (uses npm ci in the case of npm) which requires package-lock.json. ci mode is good for applications but not necessarily libraries (for the same above reasons).

I'll make an update.

@stale
Copy link

stale bot commented Aug 16, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Aug 16, 2020
@JamesHenry JamesHenry mentioned this pull request Jun 16, 2023
Merged
3 tasks
@sy-records sy-records deleted the delete-package-lock branch June 24, 2023 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhildenbiddle jhildenbiddle jhildenbiddle left review comments

@Koooooo-7 Koooooo-7 Awaiting requested review from Koooooo-7

@sy-records sy-records Awaiting requested review from sy-records

@jamesgeorge007 jamesgeorge007 Awaiting requested review from jamesgeorge007

@anikethsaha anikethsaha Awaiting requested review from anikethsaha

Assignees
No one assigned
Labels
wontfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@trusktr @anikethsaha @jhildenbiddle