Setup lerna by ang-zeyu · Pull Request #1291 · MarkBind/markbind

ang-zeyu · 2020-07-27T15:19:55Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Other, please explain:

Resolves #1285

What is the rationale for this request?
See #1285

What changes did you make? (Give an overview)
Commit order:

Move all cli files to packages/cli 5a56a69
- and update .gitignore ignore path accordingly
Add.npmignore for core package d7ca8bc
Setup lerna (gitignore update + lerna init + lerna bootstrap) 7979698
Update font awesome test files (the previous step caused some package-lock versions to change, which caused font awesome to update 605a4e2
Adapt test scripts f7c16e3
Adapt linting configs, scripts c3c22bc
- this commit also unifies the linting into one call to eslint ., and ditches separately running eslint . in all packages. This allows vue-components' eslint to properly extend the root config
Move vue-components script to root private package 823e9ec
Adapt CI and setup (install) scripts b0d33ca
Update devguide bbe4bec (wip)
- the publishing process is yet to be tested here

Is there anything you'd like reviewers to focus on?

bbe4bec 's publishing process is yet to be dummy tested

Testing instructions:

no change -- npm run test
- under the hood this now runs eslint & stylelint, then the test npm script in all packages separately

Proposed commit message: (wrap lines at 72 characters)
The cli and core packages uses relative file specifiers to link
dependencies in the mono repo. This causes a published cli package to
fail to install.

Besides site generation, the core package also houses several other
things like templates, assets and plugins, which can be moved into
separate packages for better separation of concerns.

Let's extract the main cli package into packages/cli as such, resulting
in a flat package structure.
The multi package structure implies publishing all packages
independently, which fixes the usage of relative file specifiers.

Let's also setup lerna, which is a multi-package monorepo development
and dependency management helper.
This greatly reduces the amount of work needed to manage multiple
independent packages.

marvinchin

Did some poking around and things seem to work fine! Now to see if publishing works as we expect.

Some questions/thoughts:

Do you know why the FontAwesome version was bumped? Ideally, we should retain the original version in this PR to keep these concerns separate and avoid unnecessary diffs.
I feel that we should keep the README.md in the root. This can help new devs who first visit our repo some information on the project and how to get started at first glance. Maybe we should set up a new README for cli and each of the other leaf packages, with a short explanation on what each package does.

marvinchin · 2020-07-28T10:23:33Z


 !.eslintrc.js
+
+# vue-components


Just wondering - is there any reason why we don't just use packages/vue-components here?

Some questions/thoughts:

Do you know why the FontAwesome version was bumped? Ideally, we should retain the original version in this PR to keep these concerns separate and avoid unnecessary diffs.

we previously installed the core package transitively via the cli package, so there was no package-lock there. so the latest semantic ver gets installed.

I'll get out a separate pr to update it though, then rebase this one on top of that

I feel that we should keep the README.md in the root. This can help new devs who first visit our repo some information on the project and how to get started at first glance. Maybe we should set up a new README for cli and each of the other leaf packages, with a short explanation on what each package does.

🤦‍♂️ unintentional. Will revert and add some brief readmes for cli/core after the npmignore commit during the rebase above

marvinchin · 2020-07-28T10:30:03Z

-   <popover header="Why **2** `ci` commands?" content="Our components subpackage depends on `node-sass` indirectly, which depends on `fsevents`.<br><br> `fsevents` is not needed nor compatible with Windows, and `npm ci` does not respect this since it uses `package-lock.json`.">(`ciwin:all` if you are on a Windows machine)</popover>
-   in the root folder of your cloned repo.
-1. **To bind your cloned version of MarkBind to your console** (instead of the released version of MarkBind), run `npm link` in the root folder of the cloned repo.
+1. **Install dependencies** by running <popover content="Under the hood, this calls `npm ci` and `lerna bootstrap`">`npm run setup`</popover> in the root folder of your cloned repo.


I had some issues due to existing node_modules directories in the existing packages. Do you think it's worth adding a note here for existing markbind devs to first run lerna clean before running npm run setup?

lerna clean won't work unless you have it installed globally or have run npm ci (which npm run setup does) first unfortunately. I think the devguide should be written in the perspective of new developers always though. Experienced developers could always drop an issue / ping, and have a likely have a better understanding of node already 😁

ah fair enough!

marvinchin · 2020-07-28T10:31:19Z

+  MarkBind uses <md>[lerna](https://github.com/lerna/lerna)</md>, a popular multi-package development tool, to manage it's dependencies. It is essentially a high level wrapper over node and npm's functionalities
+  </box>
+
+1. **To bind your cloned version of MarkBind to your console** (instead of the released version of MarkBind), run `npm link` in the `packages/cli` folder of the cloned repo.


Does this work for you now? I'm getting errors because our internal packages haven't been published yet.

got that too, you could get around it by temporarily removing the @markbind/core dependency and npm link then undoing it

ang-zeyu · 2020-07-28T12:04:43Z

Updated:

reverted README.md movement 502041c
add README for cli / core package 6360743
dropped font awesome commit

https://github.com/lerna/lerna/blob/master/doc/hoist.md

Also switched over to lerna bootstrap --hoist in npm run setup (package-lock changes in 502041c & d6b62c3 only), which is compatible with the current setup and should install faster + take up less space.

the second disadvantage mentioned isn't applicable either as eslint-plugin-import guards against it

marvinchin · 2020-07-29T11:44:21Z

@@ -21,9 +21,9 @@ export default {
      }
      const pages = [];
      const regexes = this.value.split(' ')
-        .filter((searchKeyword) => searchKeyword !== '')


Are these diffs intentional?

Ideally, fixing linting errors would be done in a separate PR, but if these new errors are introduced because of the changes we make to the eslint config to support lerna then I'm happy to keep it in this PR.

yup the vue eslintrc now properly extends the previous root config

marvinchin · 2020-07-30T09:04:26Z

@@ -0,0 +1,4683 @@
+{


I presume that these package-lock changes are done by lerna bootstrap? Ideally we'd be able to verify that our dependency versions are still the same, but these diffs seem a little too large to manually verify 🙁

yep, I think that if our functional tests stay the same we should be good though, since package-lock is only effective in development environment

if there are changes (like the font awesome bundles), we can then investigate them and deal with the diff in a separate pr

ang-zeyu · 2020-07-31T08:19:37Z

Updated @marvinchin, seems to work. Travis seems to be broken on all prs though, likely unrelated to the changes here

494ba03 - rename the packages with markbind- prefix
4dc983d - changed lerna publish to -> lerna publish from-git ( tells lerna to use the version commit from lerna version instead of running lerna version again )

marvinchin · 2020-07-31T14:41:17Z

This is a big PR with multiple steps. I feel that we should do a merge commit, rather than squashing the commit history here.

I also think we should add some detail to commit 494ba03 explaining why we are doing the renaming.

ang-zeyu · 2020-07-31T15:13:43Z

This is a big PR with multiple steps. I feel that we should do a merge commit, rather than squashing the commit history here.

yup that's the plan

I also think we should add some detail to commit 494ba03 explaining why we are doing the renaming.

Just received npm permissions from @yamgent, will revert back to @markbind/xx!

Development linting dependencies and configuration are duplicated throughout the packages. Let's hoist these to the root package as much as possible, removing such duplication. In doing so, some style standards are also corrected in the vue-components package. Let's fix the style violations resulting from this.

ang-zeyu · 2020-07-31T15:52:47Z

reverted to @markbind, should be ready to merge now. Thanks @marvinchin @yamgent!

marvinchin

LGTM 🌵

marvinchin reviewed Jul 28, 2020

View reviewed changes

ang-zeyu mentioned this pull request Jul 28, 2020

Bump font-awesome from 5.13.1 to 5.14.0 #1292

Merged

ang-zeyu force-pushed the setup-lerna branch from bbe4bec to 7b121f4 Compare July 28, 2020 12:00

marvinchin reviewed Jul 30, 2020

View reviewed changes

ang-zeyu force-pushed the setup-lerna branch from 7b121f4 to 4dc983d Compare July 31, 2020 08:12

ang-zeyu changed the title ~~[WIP] Setup lerna~~ Setup lerna Jul 31, 2020

ang-zeyu added this to the v2.15.0 milestone Jul 31, 2020

ang-zeyu added 9 commits July 31, 2020 23:24

Move cli related files to packages/cli

0571d8d

Add npmignore to core package

e93234b

Add readme for cli and core packages

ee667f6

Setup lerna

84767b3

Adapt tests and test scripts for lerna

6f04feb

Move vue-components shortcut scripts to root

c61a7dd

Adapt CI and setup scripts

d26481d

Adapt developer guide for lerna

b030e05

ang-zeyu force-pushed the setup-lerna branch from 4dc983d to b030e05 Compare July 31, 2020 15:47

marvinchin approved these changes Jul 31, 2020

View reviewed changes

ang-zeyu merged commit 155b918 into MarkBind:master Jul 31, 2020

ang-zeyu mentioned this pull request Sep 5, 2020

Restore colors to test output #1341

Merged

Uh oh!

Conversation

ang-zeyu commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marvinchin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ang-zeyu commented Jul 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ang-zeyu commented Jul 31, 2020

Uh oh!

marvinchin commented Jul 31, 2020

Uh oh!

ang-zeyu commented Jul 31, 2020

Uh oh!

ang-zeyu commented Jul 31, 2020

Uh oh!

marvinchin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ang-zeyu commented Jul 27, 2020 •

edited

Loading