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

Import primer-module-build to the monorepo #475

Merged
merged 30 commits into from
May 9, 2018
Merged

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Apr 27, 2018

Yesterday I ran into a bizarre issue with node-sass that appears to be fixed in a later version. Testing a new version of node-sass with Primer turns out to be really difficult without primer-module-build in the monorepo, so I went down that road. Along the way, I also:

  1. Refactored primer-module-build to:
    1. Use node-sass-import, which more intelligently resolves @import filenames and (I think!) eliminates the issue of having to npm dedupe to flatten the node_modules dependency hierarchy in just the right way.
    2. Return a Promise and execute asynchronous tasks in the correct order. (Previously there was a mix of sync and async calls, and none of the other code waited until the fs.mkdir() call to create the build directory was finished.)
    3. Write build/data.json in addition to build/index.js, which basically fixes Generate stats.json for other tools to consume primer-module-build#18.
  2. Removed stylelint-config-primer from the monorepo package.json, which is not published but could cause issues if we were to bump that package's version without also updating it there.

I'd like to test whether we can remove primer-module-build from the dotcom package.json (or specify it as "primer-module-build": "*"?) to alleviate issues that @emplums and @jonrohan ran into when testing an alpha version of primer with an updated primer-utilities version. (I think the problem there was that because primer-module-build depends on primer-utilities and the versions weren't in sync, the utilities weren't being hoisted properly.) Otherwise, this is ready to review!

Note When we're ready to release this, we'll need to bump primer-module-build to 2.0.0, as the JS API has changed.

/cc @primer/ds-core

@shawnbot shawnbot added minor release 💓collab a vibrant hub of collaboration labels Apr 27, 2018
@shawnbot shawnbot requested a review from a team April 27, 2018 22:48
@@ -20,6 +20,10 @@
"test": "npm run test-all-modules && lerna run test",
"test-all-modules": "ava --verbose tests/test-*.js"
},
"dependencies": {
"primer-module-build": "file:tools/primer-module-build",
"stylelint-config-primer": "file:tools/stylelint-config-primer"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, this was the only way that I could figure out how to "hoist" any of our packages to the top-level node_modules directory without specifying a version. It works great for CI, where things won't change, but it could very well cause issues in testing changes to the tools (primer-module-build, stylelint-config-primer) on other packages locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I broke them out into dependencies so that it was easier to see them amongst all of the other dev dependencies. 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the file: syntax only worked for local development? Does this line get replaced when we publish to npm?

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 think you're right. We don't publish the top-level package.json, though; we just use it with Lerna to hoist all of the common dev dependencies and run tests across the whole project. (The primer "mega-package" is in modules/primer.)

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh! makes sense! 👍

Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

💎 ⚡️Going to make maintenance easier

@@ -0,0 +1 @@
@import "./primer-package.scss"
Copy link
Member

Choose a reason for hiding this comment

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

🎈

package.json Outdated
"npm-run-all": "^4.0.2",
"npx": "^10.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

If we're using npx, do we still need npm-run-all in devDeps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, because npx doesn't support multiple run scripts in serial ☹️

@shawnbot
Copy link
Contributor Author

shawnbot commented May 3, 2018

Derp, gotta figure out why tests are failing now... @jonrohan, could you remove your review until then?

@jonrohan jonrohan self-assigned this May 7, 2018
@shawnbot
Copy link
Contributor Author

shawnbot commented May 9, 2018

As of d5aeca7, this PR also includes some tweaks to the scorecard test:

  1. All packages in meta/* are recognized by Lerna, and included in the test suite
  2. Because of 1., we no longer need to run the scorecard test explicitly in .travis.yml
  3. A version mismatch for execa has been fixed

@shawnbot
Copy link
Contributor Author

shawnbot commented May 9, 2018

For reference, here is the primer CSS output. I beautified it and the 10.4.0 CSS and then diffed them with:

$ diff --context=3 primer-10.4.0.css primer-alpha.css
*** /tmp/primer-10.4.0.css	2018-05-09 13:25:18.000000000 -0700
--- /tmp/primer-alpha.css	2018-05-09 13:25:37.000000000 -0700
***************
*** 3178,3184 ****
  }
  
  .tooltipped-multiline::after {
-     width: -webkit-max-content;
      width: -moz-max-content;
      width: max-content;
      max-width: 250px;
--- 3178,3183 ----
***************
*** 8852,8862 ****
  }
  
  .Popover-message--right-bottom::before,.Popover-message--left-bottom::before {
!     bottom: 24px
  }
  
  .Popover-message--right-bottom::after,.Popover-message--left-bottom::after {
!     bottom: 21px
  }
  
  @media (min-width: 544px) {
--- 8851,8861 ----
  }
  
  .Popover-message--right-bottom::before,.Popover-message--left-bottom::before {
!     bottom: 16px
  }
  
  .Popover-message--right-bottom::after,.Popover-message--left-bottom::after {
!     bottom: 17px
  }
  
  @media (min-width: 544px) {
***************
*** 9242,9248 ****
      height: 30px;
      content: " ";
      background-color: transparent;
!     background-image: linear-gradient(transparent, rgba(0,0,0,0.05));
      background-repeat: repeat-x;
      box-shadow: inset 0 -1px 0 rgba(0,0,0,0.05)
  }
--- 9241,9247 ----
      height: 30px;
      content: " ";
      background-color: transparent;
!     background-image: linear-gradient(rgba(0,0,0,0), rgba(0,0,0,0.05));
      background-repeat: repeat-x;
      box-shadow: inset 0 -1px 0 rgba(0,0,0,0.05)
  }

The .tooltipped-multiline change either comes from an updated autoprefixer and/or a smaller set of "old" WebKit browsers to support thanks to our "relative" browserslist ("last 5 versions", etc.). The Popover diff is from #465, and the linear-gradient(transparent) bit is most likely a Sass optimization.

Anyway, I think this is read to go, so I'm merging it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💓collab a vibrant hub of collaboration minor release Tag: Internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants