Merge /vue-strap into /markbind #1248

ang-zeyu · 2020-06-14T06:15:22Z

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

• [x] Other, please explain:

Resolves #411
Resolves #981

What is the rationale for this request?
Merge /vue-strap repo into /markbind

What changes did you make? (Give an overview)
I aimed for minimal changes for now, but one thing lead to another as explained in the commit messages (particularly the last one), and had to make quite a few changes still.

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

Testing instructions:

npm run build should build the minified vue-strap as per normal
npm run test/lint should pass, and the snapshot tests should also be run

Proposed commit message: (wrap lines at 72 characters)
Merge /vue-strap into /markbind

Markbind's vue component definitions are stored in the vue-strap fork.
This has some implications on development and maintainer workflow (see
issue #411 for more detail), most notably needing to manually copy the
minified bundle to test changes in /vue-strap.

An alternative was raised in issue #981 to alleviate this, that is to
make /vue-strap a scoped package. This still has several drawbacks as
compared to a complete repo merge as outlined in #411, such as needing
to run tests on both repos.

Hence, let's change the directory structure and configurations of
/markbind to accomodate the linting, testing and building processes
from /vue-strap.

ang-zeyu · 2020-06-14T07:05:08Z

@acjh one thing led to another (as explained particularly in the 2nd last (move all frontend build dependencies to /markbind) commit message), and I needed to make quite a few changes to ensure the build, linting and testing processes didn't break minimally.

Most notably I ended up deciding to remove package.json of /vue-strap (preserving the readme/license notices). Not too sure if there are benefits to retaining it at this point, but it caused some of the issues above generally because of how eslint / jest resolves some things with respect to the closest package.jsons ( details in commit messages ).

~~ci currently fails, but it works on windows systems. still investigating the cause.~~ (resolved)

Future plans:

Update dev guide after Refactor developer docs #1246
Move up webpack config files to frontend/ directory in preparation for the below point, and then change CI/CD to make the release process easier
Merge assets into the webpack build process
Setup hot/live reload for frontend development
...

acjh

Thanks for this attempt.

However, we may be moving in the wrong direction by merging the tests and dependencies.
We can provide a single helper command in package.json to run both tests, but not like this.

acjh · 2020-06-14T06:48:49Z

package.json

  ],
  "author": "Jiang Sheng <gisonrg@gmail.com>",
  "license": "MIT",
+  "homepage": "https://markbind.org/index.html",


How about just https://markbind.org?

acjh · 2020-06-14T06:50:51Z

package.json

  },
  "repository": {
    "type": "git",
    "url": "https://github.com/MarkBind/markbind.git"
  },
  "scripts": {
    "autolint": "npm run lintfix && npm run csslintfix",
+    "build": "webpack --progress --hide-modules --config  frontend/components/webpack.build.js",
+    "builddev": "webpack --progress --hide-modules --config  frontend/components/webpack.dev.js",


I don't think we should combine frontend dependencies with backend dependencies here.

How about having vuebuild, vuelint etc. that cd frontend/components and then run these there?
We can have similar commands in frontend/components/package.json for those working on only FE.

src/Page.js

.eslintignore

.stylelintignore

frontend/components/README.md

ang-zeyu · 2020-06-14T09:04:29Z

Thanks for the thorough review! Wasn't expecting one so quick. Apologies for the late reply as well, needed some time to give it more thought.

Thanks for this attempt.

However, we may be moving in the wrong direction by merging the tests and dependencies.
We can provide a single helper command in package.json to run both tests, but not like this.

To provide more context, on top of the reasons mentioned in the commit messages,
I aim to eventually streamline the whole frontend build process in light of many eventual goals:

move assets into the webpack build process as well
- allowing us do some tree shaking of bootstrap-vue
- maintain a consistent transpiling / minification / ... process to save some headaches
eventually migrate to sass stylesheets (also inside the vue component <style>s themselves) in order to facilitate Support a way to set the primary color of a site #903 using sass variables
live/hot reload for frontend ( components + assets folder )
SSR - I think this should be the eventual goal particularly given the nature of education sites (high amount of components, also why FOUC is pretty noticeable)
... probably more I forgot about here and there, but these should be the main ones =C

I haven't thought of the specifics of course, more problems will always arise when actually implementing these, but just on a quick glance I think going for a mono-package structure for now would help tremendously in these goals, since many of /vue-strap's dependencies are necessary for the above.

On the other hand, sub packages do provide a clear seperation of dependencies. On this note and the above raised points, perhaps we could eventually create a new sub-package for the whole of frontend/?
Removing the package for now (providing more flexibility) then reintroducing it later may also be easier than migrating later on, since we can't really iron out the challenges that will be faced in attaining above said goals at the present.

TLDR: perhaps we can shift back when we start to have a ton of dependencies lumped together and it's not clear what is used where.

acjh · 2020-06-14T09:49:55Z

To provide more context, on top of the reasons mentioned in the commit messages,
I aim to eventually streamline the whole frontend build process in light of many eventual goals:

Even so, we shouldn't make those changes (that should be assessed separately) in this PR.
Let's aim for a clean merge and stick to the scope of #411 that considers the existing needs.

Removing the package for now (providing more flexibility) then reintroducing it later may also be easier than migrating later on

It's very messy now. I'm not sure why you phrase it differently, you are just "migrating" now.

ang-zeyu · 2020-06-14T10:18:32Z

Even so, we shouldn't make those changes (that should be assessed separately) in this PR.
Let's aim for a clean merge and stick to the scope of #411 that considers the existing needs.

Noted. The commit organization is quite messy. Will split this into 2/3 prs. This PR should be able to retain up to the first 4 commits though

I'm not sure why you phrase it differently, you are just "migrating" now.

I guess I meant "sticking to the existing structure for now then migrating later" 😅

ang-zeyu · 2020-06-14T11:17:26Z

updated @acjh, just 5 commits now

copy and paste /vue-strap
update ignore configs (eslint, stylelint gitignore)
remove unnecessary files
update README.md
update package.json

acjh

Thanks for the changes! The diff is more reasonable now.

frontend/components/.eslintignore

acjh · 2020-06-14T12:26:24Z

frontend/components/.eslintignore

+src/Tab.vue
+src/TabGroup.vue
+src/Tabset.vue
+src/Thumbnail.vue


This should go under # MarkBind/vue-strap; wrongly added in MarkBind/vue-strap#107.

Actually, I ignored .vue files in MarkBind/vue-strap#29 to avoid large code style changes (especially those under # yuche/vue-strap) — for files that we barely touch, we tolerate the style for cleaner git annotate. New files should be linted and we can consider linting those under # MarkBind/vue-strap.

Actually, I ignored .vue files in MarkBind/vue-strap#29 to avoid large code style changes

I'll open an issue for this sometime, but it's rather unrelated at the moment.
Perhaps we could use git annotate to see the % of lines changed to decide which to unignore, but as we migrate more components to bootstrap-vue this shouldn't be necessary 😄

frontend/components/README.md

frontend/components/package.json

acjh · 2020-06-14T13:01:51Z

frontend/components/package.json

+    "lint": "./node_modules/.bin/eslint . --ext .js,.vue; exit 0",
+    "lintwin": "./node_modules/.bin/eslint . --ext .js,.vue",
+    "lintfix": "./node_modules/.bin/eslint . --fix --ext .js,.vue"
+  },


Do we want to add helper commands (e.g. vuebuild, vuelint, lintall) that cd frontend/components and then run these in the "scripts" of the root package.json?
Or do we plan to do it in a separate PR?

if we go with removing package.json and use nested eslintrcs that won't be necessary, but either way a separate PR would be better.
Right now i'm aiming for "copy and paste + make ci work + edit whatever documentation dosen't make sense anymore" (I'll add in devdocs as well once #1246 gets merged)

package.json

frontend/components/README.md

ang-zeyu · 2020-06-14T15:36:02Z

@acjh ready for another review, resolved your comments and updated with a 6th commit updating the dev guide as well

ang-zeyu · 2020-06-14T16:08:13Z

@acjh @marvinchin on the multi package topic, I was curious how much larger projects handled the amount of subpackages they had - and found this https://lerna.js.org/

This potentially resolves all the concerns I had with duplicated dependencies / npm installs I had previously. I've yet to test the specifics though (like whether jest, babel-jest, etc. plays nice with this which gave quite a few issues on its own already) *crosses fingers 🤞*

It even has some features like lerna publish to publish multiple packages with the same version simultaneously, which can help improve multi package release processes

If all goes well (not having compatibilty issues with nested babel configs, etc.), I feel this would be the next logical step to take before attempting any more of the plans #1248 (comment).

What do you think of it?

On second thought, I'm still not sure if there's much point in maintaining a seperate package just for the vue components though. To reiterate,

many of the dependencies in it will eventually by necessary in markbind as well.
Segregation of test, linting configs can still be achieved by retaining the nested configutation files.
While having something like lerna makes managing monorepos easier, it still is additional maintainment overhead.
it's a core part of markbind, and not something I would see being reused. (on the premise that subpackages usually provide some optional functionalities (e.g. plugins))
- as another example, separation of markbind-cli from markbind is a good candidate (although we dont really make use of this right now - page / site.js being in /markbind-cli, this could be really useful if we eventually have markbind-ui ).

acjh

Suggested some improvements for phrasing and consistency.

docs/devGuide/design.md

frontend/components/README.md

acjh · 2020-06-14T17:36:26Z

frontend/components/package.json

+    "webpack-cli": "^3.3.11",
+    "webpack-dev-server": "^3.10.3"
+  },
+  "scripts": {


Let's further adjust the order based on https://docs.npmjs.com/files/package.json.

"name": "vue-strap", "version": "2.14.1", "description": "Bootstrap components built with Vue.js", + "keywords": [ + "homepage": "https://markbind.org", + "license": "MIT", + "files": [ "main": "dist/vue-strap.js", "repository": { - "homepage": "https://markbind.org", - "license": "MIT", + "scripts": { "dependencies": { - "files": [ - "keywords": [ "devDependencies": { - "scripts": {

will do the ones for markbind/markbind-cli in a seperate PR as well

package.json

acjh · 2020-06-14T18:02:05Z

What do you think of it?

I don't think we currently need Lerna. MarkBind has way fewer packages than several of the repos — only 3 (we haven't adopted my npm plugin proposal) — and we currently only publish one.

On second thought, I'm still not sure if there's much point in maintaining a seperate package just for the vue components though. To reiterate,

If we develop a feature that adds significant value and absolutely requires combining the dependencies, then we will cross that bridge.

Separation of concerns has value. It's not something we should abandon for convenience.

ang-zeyu · 2020-06-15T11:19:53Z

Thanks for the thorough suggestions! @acjh Rectified accordingly

Separation of concerns has value.

Definitely, I just don't think that having the formalities of a package.json indicating a subpackage helps much with this. SoC should be primarily achieved with when looking at the code changes during PR reviews imo. A semantic directory structure should also help in achieving SoC in place of package.json.
Simply having a package.json dosen't add any value to me in this sense, only overhead in dependency management.

That said, I don't have a lot of experience working with multi package environments; I'm willing to see where this goes for now, if need be we can always eliminate it in the future.

I don't think we currently need Lerna. MarkBind has way fewer packages than several of the repos — only 3 (we haven't adopted my npm plugin proposal) — and we currently only publish one.

👍, could reconsider if we have more / actually restructure markbind/markbind-cli for their respective purposes ( reason being we might want to move unit tests in there, and want to avoid running 3 npm run tests )

I just found out that npm install <folder> hoists dependencies as well. Minimally we should use that to avoid duplicated dependency bloat. (in fact we should probably be using this for markbind/markbind-cli as well to avoid stating dependencies in markbind-cli which aren't even directly used by it)

acjh

Suggested some touch-ups.

acjh · 2020-06-16T13:28:41Z

frontend/components/package.json

+    "vue",
+    "markbind"
+  ],
+  "homepage": "https://markbind.org",


should this be https://markbind.org/userGuide/usingComponents.html?

hmm might be better to just use markbind.org in case we shift usingComponents next time. it contains some irrelevant things like the page/sitenav too

acjh · 2020-06-16T13:31:54Z

frontend/components/package.json

+    "LICENSE",
+    "README.md",
+    "CHANGELOG.md",
+    "src",


seems there was never CHANGELOG.md and it's not necessary to package src.

frontend/components/package.json

Markbind's vue component definitions are stored in the vue-strap fork. This has some implications on development and maintainer workflow (see issue MarkBind#411 for more detail), most notably needing to manually copy the minified bundle to test changes in /vue-strap. An alternative was raised in issue MarkBind#981 to alleviate this, that is to make /vue-strap a scoped package. This still has several drawbacks as compared to a complete repo merge as outlined in MarkBind#411, such as needing to run tests on both repos. Hence, let's prepare the directory structure of the vue-strap fork to allow for a clean merge into the /markbind repo, preserving the commit history of /vue-strap.

Vue-strap enables coverage reporting for snapshot tests. The linting and jest configurations need to be updated as well to isolate linting and tests to their original repos as well. Let's add the coverage folder to .gitignore to avoid large coverage diffs. Let's also update the eslint, stylelint and jest configurations to ignore the appropriate folders. Some vue component no longer exist that are still in the .eslintrc file as well. Let's remove these.

With the /vue-strap fork now merged, let's update README.md to reflect the origins of the project, and attribute the components modified from /vue-strap.

Let's update package.json to remove some unneeded fields leftover from the fork, such as browserify, and to reflect the new homepage.

acjh · 2020-06-17T12:32:58Z

Btw, I suggest using git commit --fixup + git rebase --autosquash flow instead of constantly force-pushing for easier review with clearer diff.

https://blog.sebastian-daschner.com/entries/git-commit-fixup-autosquash

ang-zeyu · 2020-06-17T13:04:40Z

Btw, I suggest using git commit --fixup + git rebase --autosquash flow instead of constantly force-pushing for easier review with clearer diff.

https://blog.sebastian-daschner.com/entries/git-commit-fixup-autosquash

thanks for the review and tip!

I don't quite get the force pushing though - since the commits after the fixup-ed commit have their hashes changed, wouldn't that still require a force push? (or do you mean to do the git rebase --autosquash part after the reviewer has approved the fixup commit?)

acjh · 2020-06-17T13:20:21Z

Yup.

ang-zeyu changed the title ~~Merge /vue-strap~~ [WIP] Merge /vue-strap into /markbind Jun 14, 2020

ang-zeyu force-pushed the merge-vue-strap branch from 43a01ba to ecce674 Compare June 14, 2020 07:54

ang-zeyu changed the title ~~[WIP] Merge /vue-strap into /markbind~~ Merge /vue-strap into /markbind Jun 14, 2020

ang-zeyu force-pushed the merge-vue-strap branch 2 times, most recently from 4eaa1f2 to 04e7fab Compare June 14, 2020 07:59

acjh reviewed Jun 14, 2020

View reviewed changes

ang-zeyu force-pushed the merge-vue-strap branch 2 times, most recently from 7e1577c to b0b5788 Compare June 14, 2020 11:12

acjh reviewed Jun 14, 2020

View reviewed changes

ang-zeyu force-pushed the merge-vue-strap branch from b0b5788 to 46f7594 Compare June 14, 2020 15:29

acjh reviewed Jun 14, 2020

View reviewed changes

ang-zeyu force-pushed the merge-vue-strap branch 2 times, most recently from 5cf6059 to d69cff5 Compare June 15, 2020 11:17

acjh reviewed Jun 16, 2020

View reviewed changes

ang-zeyu added 2 commits June 17, 2020 18:16

ang-zeyu force-pushed the merge-vue-strap branch from d69cff5 to 8d7e4a8 Compare June 17, 2020 10:28

ang-zeyu added 4 commits June 17, 2020 18:36

Remove unnecessary files from /vue-strap fork

ab459e5

Update README.md of frontend/components

f75a056

With the /vue-strap fork now merged, let's update README.md to reflect the origins of the project, and attribute the components modified from /vue-strap.

Update package.json of frontend/components

cec49b4

Let's update package.json to remove some unneeded fields leftover from the fork, such as browserify, and to reflect the new homepage.

Update developer guide for /vue-strap merge

0d2ee5a

ang-zeyu force-pushed the merge-vue-strap branch from 8d7e4a8 to 0d2ee5a Compare June 17, 2020 10:37

acjh approved these changes Jun 17, 2020

View reviewed changes

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

ang-zeyu merged commit e000688 into MarkBind:master Jun 17, 2020

acjh mentioned this pull request Jun 27, 2020

Hoist markbind package dependencies #1253

Merged

ang-zeyu mentioned this pull request Jul 26, 2020

v2.15.0 fails to install entirely #1285

Closed

kaixin-hc mentioned this pull request Feb 14, 2024

Tooltips seem to add an extra space in front #2419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge /vue-strap into /markbind #1248

Merge /vue-strap into /markbind #1248

ang-zeyu commented Jun 14, 2020 •

edited

Loading

ang-zeyu commented Jun 14, 2020 •

edited

Loading

acjh left a comment

acjh Jun 14, 2020

acjh Jun 14, 2020

ang-zeyu commented Jun 14, 2020 •

edited

Loading

acjh commented Jun 14, 2020

ang-zeyu commented Jun 14, 2020

ang-zeyu commented Jun 14, 2020

acjh left a comment

acjh Jun 14, 2020

ang-zeyu Jun 14, 2020

acjh Jun 14, 2020

ang-zeyu Jun 14, 2020

ang-zeyu commented Jun 14, 2020

ang-zeyu commented Jun 14, 2020 •

edited

Loading

acjh left a comment

acjh Jun 14, 2020

ang-zeyu Jun 15, 2020

acjh commented Jun 14, 2020

ang-zeyu commented Jun 15, 2020

acjh left a comment

acjh Jun 16, 2020

ang-zeyu Jun 17, 2020

acjh Jun 17, 2020

acjh Jun 16, 2020

acjh commented Jun 17, 2020

ang-zeyu commented Jun 17, 2020

acjh commented Jun 17, 2020

Merge /vue-strap into /markbind #1248

Merge /vue-strap into /markbind #1248

Conversation

ang-zeyu commented Jun 14, 2020 • edited Loading

ang-zeyu commented Jun 14, 2020 • edited Loading

acjh left a 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 14, 2020 • edited Loading

acjh commented Jun 14, 2020

ang-zeyu commented Jun 14, 2020

ang-zeyu commented Jun 14, 2020

acjh left a 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 14, 2020

ang-zeyu commented Jun 14, 2020 • edited Loading

acjh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acjh commented Jun 14, 2020

ang-zeyu commented Jun 15, 2020

acjh left a 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

acjh commented Jun 17, 2020

ang-zeyu commented Jun 17, 2020

acjh commented Jun 17, 2020

ang-zeyu commented Jun 14, 2020 •

edited

Loading

ang-zeyu commented Jun 14, 2020 •

edited

Loading

ang-zeyu commented Jun 14, 2020 •

edited

Loading

ang-zeyu commented Jun 14, 2020 •

edited

Loading