-
Notifications
You must be signed in to change notification settings - Fork 229
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
Convert to monorepo using lerna.js #153
Conversation
'adjoining-classes': SUIT is ok with adjoining classes, and requires them for component state. 'compatible-vendor-prefixes': this rule isn't useful and doesn't take into account browser market share (so it keeps warning about vendor-prefixes that you don't want). The framework also favours using a preprocessor to handle and update vendor prefixes automatically.
Everything is covered in the generated component's README
Doesn't provide enough value and inadequate plugin support (for things like code style and not warning about variable syntax).
* Add npm support. * Include latest build tools. * Correct names in manifests. * Update generated test file. * Update code style in generated component.
…ed-pkg Update modules in generated package
Rewrite and Change in generator API Fixes suitcss#4
Rename the package.json template to be extensionless Fixes suitcss#9
Fix: prefix modulePackageName with suitcss-
* package.json template: * Update dependencies * Make 0.1.0 the default ver * Add license * Fix indentation in the lib/*.css templates
Update package.json template
Add year and author name to package.json and LICENSE.md templates
Fix documentation example
Includes latest change to CSS variables syntax.
Updating stylelint to v8
Update stylelint to v9
* growl contained a critical security issue (https://www.npmjs.com/advisories/146) * nsp has been replaced by npm audit * prepublish hook is called by lerna bootstrap
* Prefers yarn.lock over package-lock.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @mlnmln !! I took a look at the PR and this is a first round of feedback. Hopefully next week I will have some time to clone and play with it.
package.json
Outdated
@@ -12,6 +12,7 @@ | |||
"suitcss-utils": "^3.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently suitcss/suit
is a package on its own. Is it necessary or does it make sense to create a pkg in packages/suit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary, but I would recommend moving it to packages/suit
. That way the docs could stay at root and the code would be in its own folder.
@@ -1,4 +1,3 @@ | |||
bower_components | |||
build | |||
components |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you didn't remove this from other packages. Should we do for all of them or for none?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lerna version
was (wrongfully?) complaining that some packages would be under gitignore, e.g. components-button
. It think it would be safe to remove it from all packages. component
itself is deprecated for more than 3 years now.
@@ -0,0 +1,4 @@ | |||
language: node_js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that with the monorepo now we need a centralized .travis.yml
config and maybe even a centralized test runner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but I would track that in a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep makes sense, thanks!
@@ -0,0 +1,4 @@ | |||
language: node_js | |||
sudo: false | |||
node_js: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should normalize this field too. I'd pick node 8 and 10, 6 is going out the LTS maintenance in April.
|
||
* [npm](http://npmjs.org/): `npm install suitcss-base` | ||
* [Component(1)](http://github.com/component/component): `component install suitcss/base` | ||
* [Download](https://github.com/suitcss/base/releases) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably we should remove these download links
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
## Installation | ||
|
||
* [npm](http://npmjs.org/): `npm install suitcss-base` | ||
* [Component(1)](http://github.com/component/component): `component install suitcss/base` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this keep working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not even working right now. :)
mlnmln@localhost ~/d/component> component install suitcss/base
installed : suitcss/[email protected] in 2099ms
fatal : no remote found for dependency "necolas/[email protected]". Visit http://component.github.io/troubleshooting for help.
having said that. for as long as we keep the original repo and archive it, . component installation should still work.
@giuseppeg Thanks for the feedback, I added my points. My main goal now, would be to have a shared understanding of scope / task breakdown. This PR allows publishing new versions for each package through lerna.js, (hopefully) without breaking anything. I would strongly recommend splitting any other features (shared test runner, travis config etc., ) into separate issues, unless you think they are critical for landing the PR. Otherwise we can work on this indefinitely. |
I think this looks good!
I agree with this. This work will give us the push to get carry on with other outstanding issues and we can refine the lerna setup as we go. Great stuff @mlnmln. One question regarding the root level |
Glad to hear.
For as long as we use We would only bump the But to have that more clearly separated I think we should take action on #153 (comment) Then we essentially would have a (private) That package then would take care of shared dev dependencies etc in the long run, but is not installable on its own via npm. It relatively easy and I could do it during the next couple of days. What do you think? |
That seems a logical approach to me, yes |
|
@mlnmln I guess that for now we can duplicate it |
works for me. |
@simonsmith I just pushed a @mlnmln thank you a lot for your help and patience :) |
Looks great to me. Merging |
@simonsmith @giuseppeg
nsp
(bc of reported security issue,lerna bootstrap
would not finish otherwise).repo
,homepage
andbugs
in all packages.Use
lerna bootstrap
to install all packagesTo test lerna versioning
lerna version patch --yes --no-push
to test locally before doing anything on remote.To publish
npm login
, thenlerna publish patch --yes
This results in the following version bumps, everything should stay 100% backwards compatible.
This PR is limited in scope intentionally and should be a good foundation for further development work with a monorepo approach.
#151