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

chore: migrate from lerna bootstrap to npm workspaces #2094

Merged
merged 8 commits into from
Jun 24, 2023

Conversation

JamesHenry
Copy link
Contributor

Summary

Hi Docsify Folks 👋

I'm James, you might possibly know me already from having created https://github.com/typescript-eslint/typescript-eslint or my work on some other popular open-source tools over the years such as ESLint, Prettier and Babel. Right now I've got my Lerna maintainer hat on 🐉

In lerna v7 we removed lerna boostrap and other legacy package management commands by default. You can read about the full context if you are interested here: https://lerna.js.org/docs/legacy-package-management

This PR updates to latest lerna v7 and migrates the codebase to use npm workspaces instead of lerna boostrap.

You no longer need to run npm install in nested directories in CI, as npm workspaces handles everything natively from the root.

Please let me know if you have questions!

For any code change,

  • Related documentation has been updated if needed
  • N/A [ ] Related tests have been updated or tests have been added

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

Related issue, if any:

You could close #1216 and #2092 in favor of this PR

@vercel
Copy link

vercel bot commented Jun 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docsify-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 24, 2023 9:00pm

@@ -26,10 +26,6 @@ jobs:
- name: Lint
run: npm run lint

- name: Verify dependencies [server-renderer]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer needed, npm ci natively understands workspaces and will install everything from the root

Copy link
Member

Choose a reason for hiding this comment

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

This is no longer needed, npm ci natively understands workspaces and will install everything from the root

Hi James, we actually have 2 repos and the renderer one is the subfolder of root but not a module. So we need run ci seperated for them.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @Koooooo-7, can you describe the issue here more?

What James is mentioning is that the above step,

        run: npm ci --ignore-scripts

also installs dependencies for server-renderer.

Copy link
Member

Choose a reason for hiding this comment

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

I think we're fine: I don't think many people (if any?) are using SSR, and our current dev:ssr script is even broken like so:

$ npm run dev:ssr
/Users/trusktr/src/docsifyjs+docsify/packages/docsify-server-renderer/build.js:8
var fetch = _interopDefault(require('node-fetch'));
                            ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/trusktr/src/docsifyjs+docsify/packages/docsify-server-renderer/node_modules/node-fetch/src/index.js from /Users/trusktr/src/docsifyjs+docsify/packages/docsify-server-renderer/build.js not supported.
Instead change the require of index.js in /Users/trusktr/src/lume+lume+develop/packages/docsifyjs+docsify/packages/docsify-server-renderer/build.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/trusktr/src/lume+lume+develop/packages/docsifyjs+docsify/packages/docsify-server-renderer/build.js:8:29)
    at Object.<anonymous> (/Users/trusktr/src/lume+lume+develop/packages/docsifyjs+docsify/server.js:6:20) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v18.13.0
lib/plugins/emoji.js
ERROR: "serve:ssr" exited with 1.

I a new issue which should address this: #2102

I think it would be ok to merge this then fix any issues we may find, if we even care about them.

Even better: I recommend we just delete the SSR stuff because it is a pain to maintain that none of us have time for. In fact, install Docsify plugins and SSR breaks very easily. It is in a very alpha quality that we aren't using ourselves and honestly haven't cared for, so I think deleting SSR, and improving our code base (move to ESM, clean up some logic), would help us better prepare for thinking about the proper way to implement SSR.

If we decide to then restore SSR, I think we should convert to a Docsify component (f.e. using Solid.js, React, Vue, Svelte, or etc) and let a tool like Astro (maintained by a team of people who think about SSR and SSG all day every day, and are making the latest advancements in the area) give us the best SSR and SSG setup that we can simply ride upon. This will be very powerful, and without us having our own SSR implementation that doesn't work well and is far from complete.

With this in mind, I would be ok to roll with this, as I tested non-SSR Docsify and the dev experience is working great.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 16, 2023

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 20896cf:

Sandbox Source
docsify-template Configuration

@JamesHenry
Copy link
Contributor Author

Thanks a lot for reviewing my PR folks!

I thought it might be easier to respond via a video.

I am sorry for the poor quality, Github forced me to make the file under 10MB, so I had to compress it quite a bit.

Brave.Browser.2.mp4

Please could you grant permission for the CI workflow to run?

Copy link
Member

@sy-records sy-records left a comment

Choose a reason for hiding this comment

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

Thank you for your explanation. I approved the operation of CI.

@JamesHenry
Copy link
Contributor Author

@sy-records Thanks! Looks like CI is green which is great. So to finish this off you want me to update the README to remove the instructions for running npm run build:ssr and just have it be:

npm install && npm run dev

Is that right?

@sy-records
Copy link
Member

yep

@JamesHenry
Copy link
Contributor Author

@sy-records Thanks, done! You'll need to approve the CI run again please

@JamesHenry
Copy link
Contributor Author

Looks like we're all good now!

@trusktr
Copy link
Member

trusktr commented Jun 23, 2023

This is awesome. Thanks @JamesHenry!

Copy link
Member

@sy-records sy-records left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you! @JamesHenry

@sy-records sy-records requested review from trusktr and Koooooo-7 June 23, 2023 07:36
@trusktr trusktr merged commit 4b9b464 into docsifyjs:develop Jun 24, 2023
@trusktr
Copy link
Member

trusktr commented Jun 24, 2023

@JamesHenry Quick question: we gitignored the package-lock.json in packages/docsify-server-renderer. What are your thoughts on that?

@trusktr trusktr mentioned this pull request Jun 25, 2023
9 tasks
@JamesHenry
Copy link
Contributor Author

@trusktr Yeah that makes sense to protect against folks accidentally recreating it and confusing things!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants