Hoist markbind package dependencies #1253

ang-zeyu · 2020-06-15T12:08:30Z

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

• [x] Other, please explain: Dependency maintainability

What is the rationale for this request?

Reduce dependency management overhead in managing the duplicated dependencies in both packages, many of which markbind-cli does not use as well.
Update all relative markbind imports to use the markbind package import instead to discourage writing long relative imports

What changes did you make? (Give an overview)

removed all dependencies which are being used in markbind but not markbind-cli, then running a `npm install && npm install src\lib\markbind
added a missing markdown-it dependency for markbind
use the patched version of markdown from the markbind package for one of the tests instead of markdown-it to remove the markdown-it dependency; We have quite a few patches / added plugins for the behaviour in markdown-it which may interfere with the the plugin being tested itself as well.
use package imports for markbind-cli

Proposed commit message: (wrap lines at 72 characters)
Hoist markbind core package dependencies

Markbind-cli lists many packages under as its dependencies which are
used by the core markbind package but not markbind-cli.
Importing files from markbind from markbind-cli also requires writing
long relative imports.

Let's use npm install instead to hoist these packages to
markbind-cli's node_modules.
Let's also use package imports for importing the core markbind library
files.

ang-zeyu · 2020-06-16T11:29:07Z

.eslintignore

@@ -1,4 +1,5 @@
 *.min.*
+node_modules


added this as there are still some unhoisted dependencies (moment) that will be installed in markbind (lib)

ang-zeyu · 2020-06-16T11:29:45Z

__mocks__/fs-extra-promise.js

@@ -135,7 +135,6 @@ fs.copySync = (src, dest) => {
 */
 fs.readJsonSync = filePath => JSON.parse(fs.readFileSync(filePath, 'utf8'));

-


these occur due to dependency upgrades of eslint

ang-zeyu

@acjh what do you think of this?
The downside is that you'd have to run npm install src/lib/markbind (provided as npm run install:markbind) when upgrading the core package's dependencies, something obscure (as opposed to cd src/lib/markbind && npm install) I noted in the devGuide.

I noticed node's hoisting feature does not hoist devDependencies as well, which is somewhat of a bummer for deduplicating markbind-cli - frontend/components dev dependencies later on (in another PR).

ang-zeyu · 2020-06-26T14:13:02Z

package.json

    "install:components": "cd frontend/components && npm install",
+    "install:markbind": "npm install src/lib/markbind",


hence, for example if you only updated the markbind core package's dependencies, simply run install:markbind.
If multiple packages had their dependencies upgraded, run install:all instead. (documented under workflow under dev guide)

docs/devGuide/workflow.md

src/Page.js

test/unit/markdown-it-icons.test.js

src/Site.js

src/plugins/default/markbind-plugin-footnotes-popovers.js

test/unit/parser.test.js

ang-zeyu · 2020-06-27T04:08:28Z

Thanks for the review @acjh! Pushed the relevant fixups

acjh · 2020-06-27T16:22:50Z

src/lib/markbind/package.json

@@ -27,6 +27,7 @@
    "markdown-it-emoji": "^1.4.0",
    "markdown-it-imsize": "^2.0.1",
    "markdown-it-ins": "^2.0.0",
+    "markdown-it-linkify-images": "^1.1.1",


Shall we adjust the order of the sections in both package.json in this PR?
Or will it significantly mess up the diff since we're modifying packages too?

Ref: #1248 (comment)

Hmm perhaps the one that would move src/lib/markbind into packages/markbind? Would be more ~~suitable~~ relevant I think
The diffs wouldn't be too big if we do it here either though

I'm ok with either, if it doesn't significantly mess up the diff in this PR.

👍 I'll give it a try, will stash it if it's too much

The diffs seem ok (Fixup hoist markbind core package dependencies commit); I extracted out the jest config into a separate file for consistency as well

4da36b4 should be its own commit instead of a fixup.
Otherwise it messes up the diff since it's no longer obvious that we removed several packages.

jest.config.js

src/lib/markbind/package.json

acjh · 2020-06-28T04:00:05Z

src/lib/markbind/package.json

@@ -27,6 +27,7 @@
    "markdown-it-emoji": "^1.4.0",
    "markdown-it-imsize": "^2.0.1",
    "markdown-it-ins": "^2.0.0",
+    "markdown-it-linkify-images": "^1.1.1",


4da36b4 should be its own commit instead of a fixup.
Otherwise it messes up the diff since it's no longer obvious that we removed several packages.

Markbind-cli lists many packages under as its dependencies which are used by markbind but not markbind-cli. Let's use npm install <folder> instead to hoist these packages to markbind-cli's node_modules.

The package.json files of the various packages are not standardised in their ordering and content. Let's add and reorder the appropriate fields to do so.

Fix up use package imports for markbind core library Fix up use package import for markbind

In hoisting markbind dependencies, npm install also caused some of eslint's dependencies' versions in package-lock.json to increase. Let's update the source files affected by npm run lintfix.

Fix up add updating dependencies section to devGuide

ang-zeyu · 2020-06-28T04:35:14Z

4da36b4 should be its own commit instead of a fixup.

Done, the new order:

ang-zeyu · 2020-06-28T05:00:53Z

Thanks again for the review @acjh! 😄

Will be dealing with the packages/markbind next, on that note, should we:

move frontend/components into packages/components as well?
improve markbind/markbind-cli separation by moving assets into the markbind core package packages/markbind

an alternative to (2) being to create another package for assets, but it seems a little awkward having a package just for that

acjh · 2020-06-28T05:19:11Z

Will be dealing with the packages/markbind next, on that note, should we:

move frontend/components into packages/components as well?

Sounds good.

Alternatives for the record:

src/lib/markbind → packages/markbind or packages/core
frontend/components → packages/components or packages/vue-components

improve markbind/markbind-cli separation by moving assets into the markbind core package packages/markbind

an alternative to (2) being to create another package for assets, but it seems a little awkward having a package just for that

Looks like asset/ is only used by Site.js, which is currently in markbind-cli?

ang-zeyu · 2020-06-28T05:27:38Z

Alternatives for the record:

I'll go with the latter options, they look more semantic 😁

Looks like asset/ is only used by Site.js, which is currently in markbind-cli?

yup, with the eventual goal being to move Site/Page.js + all other non-cli functionality into packages/markbind or packages/core. We could deal with this in one go in a later PR though, suggesting it now since we're touching on "frontend"

acjh · 2020-06-28T05:33:38Z

Looks like asset/ is only used by Site.js, which is currently in markbind-cli?

yup, with the eventual goal being to move Site/Page.js + all other non-cli functionality into packages/markbind or packages/core. We could deal with this in one go in a later PR though, suggesting it now since we're touching on "frontend"

Let's do it in a later PR.

ang-zeyu force-pushed the hoist-dependencies branch 2 times, most recently from 928e2d2 to 596fdd5 Compare June 16, 2020 11:24

ang-zeyu commented Jun 16, 2020

View reviewed changes

ang-zeyu force-pushed the hoist-dependencies branch 3 times, most recently from e07e7e6 to 55718c5 Compare June 26, 2020 14:00

ang-zeyu closed this Jun 26, 2020

ang-zeyu reopened this Jun 26, 2020

ang-zeyu force-pushed the hoist-dependencies branch from 55718c5 to 7075664 Compare June 26, 2020 14:08

ang-zeyu commented Jun 26, 2020

View reviewed changes

acjh reviewed Jun 26, 2020

View reviewed changes

acjh reviewed Jun 27, 2020

View reviewed changes

ang-zeyu force-pushed the hoist-dependencies branch from 37b17d3 to 32c0940 Compare June 28, 2020 02:43

acjh reviewed Jun 28, 2020

View reviewed changes

ang-zeyu added 5 commits June 28, 2020 12:33

Hoist markbind core package dependencies

d5cfa47

Markbind-cli lists many packages under as its dependencies which are used by markbind but not markbind-cli. Let's use npm install <folder> instead to hoist these packages to markbind-cli's node_modules.

Standardise package.json fields

d55d52d

The package.json files of the various packages are not standardised in their ordering and content. Let's add and reorder the appropriate fields to do so.

Use package imports for markbind core library

8e25bd5

Fix up use package imports for markbind core library Fix up use package import for markbind

Update source files affected by dependency upgrade

e0f289f

In hoisting markbind dependencies, npm install also caused some of eslint's dependencies' versions in package-lock.json to increase. Let's update the source files affected by npm run lintfix.

Add updating dependencies section to devGuide

6c734d7

Fix up add updating dependencies section to devGuide

ang-zeyu force-pushed the hoist-dependencies branch from 32c0940 to 6c734d7 Compare June 28, 2020 04:33

acjh approved these changes Jun 28, 2020

View reviewed changes

ang-zeyu added this to the v2.15.0 milestone Jun 28, 2020

ang-zeyu merged commit a13810c into MarkBind:master Jun 28, 2020

ang-zeyu mentioned this pull request Jun 28, 2020

Fix nunjucks path resolving #1263

Merged

ang-zeyu mentioned this pull request Jun 28, 2020

Refactor package directories #1265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hoist markbind package dependencies #1253

Hoist markbind package dependencies #1253

ang-zeyu commented Jun 15, 2020

ang-zeyu Jun 16, 2020

ang-zeyu Jun 16, 2020

ang-zeyu left a comment

ang-zeyu Jun 26, 2020

ang-zeyu commented Jun 27, 2020

acjh Jun 27, 2020

ang-zeyu Jun 27, 2020

acjh Jun 27, 2020

ang-zeyu Jun 27, 2020

ang-zeyu Jun 28, 2020

acjh Jun 28, 2020

acjh Jun 28, 2020

ang-zeyu commented Jun 28, 2020

ang-zeyu commented Jun 28, 2020 •

edited

Loading

acjh commented Jun 28, 2020

ang-zeyu commented Jun 28, 2020

acjh commented Jun 28, 2020

		@@ -135,7 +135,6 @@ fs.copySync = (src, dest) => {
		*/
		fs.readJsonSync = filePath => JSON.parse(fs.readFileSync(filePath, 'utf8'));

		"install:components": "cd frontend/components && npm install",
		"install:markbind": "npm install src/lib/markbind",

Hoist markbind package dependencies #1253

Hoist markbind package dependencies #1253

Conversation

ang-zeyu commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu commented Jun 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu commented Jun 28, 2020

ang-zeyu commented Jun 28, 2020 • edited Loading

acjh commented Jun 28, 2020

ang-zeyu commented Jun 28, 2020

acjh commented Jun 28, 2020

ang-zeyu commented Jun 28, 2020 •

edited

Loading