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

Fix primerize, add "fresh" run-script, etc. #597

Merged
merged 27 commits into from
Nov 9, 2018
Merged

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Nov 8, 2018

This PR does a couple of things to ease our workflow:

  • Improves the npm run clean script under the assumption that you'll run npm install afterward.

  • Introduces npm run fresh (so fresh and so clean!) to clean and reinstall in one go, and suggests that in the development docs.

  • Fixes stylelint selector-no-utility has a runtime primer-utilities dependency #592 by removing the runtime dependency on primer-utilities (moving that package from dependencies to devDependencies, which means that it won't be installed in dotcom). At prepare time, stylelint-selector-no-utility now builds a classes.json file (which is not committed to git, but you can see it published), which it loads at runtime.

    The upshot of this is that installing primer@10.10.0 will just work, rather than preventing primer-support and/or primer-utilities from being deduped by npm.

  • Disables the stylelint primer/selector-no-utility rule when linting in primer-utilities. (Note: I honestly don't know how this was passing before, but this seems like a reasonable change regardless.)

  • Removes and disables module-level (modules/primer-utilities, tools/primer-module-build) package-lock.json, which appear to have been a source of some lerna bootstrap issues.

  • Replaces chalk with colorette, which is faster and has 0 dependencies.

TODO

  • confirm that this builds on Travis
  • if Travis build times get dramatically longer, try reintroducing the module-level package-lock.json
  • Ensure that classes.json gets published
  • Test it in dotcom with primerize and bin/npm install primer@10.10.0-alpha.<sha>

@shawnbot shawnbot changed the title [WIP] Fix primerize, add "fresh" run-script, etc. Fix primerize, add "fresh" run-script, etc. Nov 9, 2018
Copy link
Contributor

@emplums emplums left a comment

Choose a reason for hiding this comment

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

Just left a couple of questions, otherwise this looks good!

@@ -33,9 +33,6 @@
"dependencies": {
"primer-support": "4.6.1"
},
"peerDependencies": {
"primer-buttons": "2.6.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be moved into dependencies if it's removed from peerDeps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided to kill the peer dependencies because they weren't adding any value. In both of these cases, the peer dependencies weren't "runtime" dependencies in Sass land.

package.json Outdated Show resolved Hide resolved
@@ -1,13 +1,15 @@
{
"private": true,
"scripts": {
"prefresh": "npm run clean || echo 'clean failed; no worries!'",
"fresh": "npm install",
Copy link
Contributor

Choose a reason for hiding this comment

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

why not make fresh npm run clean && npm install instead of using prefresh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tended to break things up in pre and post scripts whenever possible because it makes them easier to test: in this case, we can run npm run prefresh to test that "phase" without having to hunt for the right part of the output before npm install. But yeah, it's not like it's doing anything crazy, so I could collapse these. I think we'd need to do npm run clean; npm install, since npm run clean will fail if you haven't installed previously (or don't have node_modules).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to leave this as-is for now. I think using pre and post scripts do make it easier to read the individual "tasks" without having to parse out the && or || operators in your head, and they're easier for folks without shell experience to understand.

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.

2 participants