Refactor package manager detection, add support for detecting `pnpm` #876

askoufis · 2023-08-24T01:25:26Z

The primary change in this PR is refactoring how we detect package managers. This code is contained within the new packageManager lib, leveraging @manypkg/find-root.

During this refactor I found a few bugs too. They're detailed within the changesets, but I'll add comments explaining them a bit too.

There will be a followup PR after this one that will add pnpm support for sku init, likely via CLI flag. That change will be released alongside the changes in this PR.

changeset-bot · 2023-08-24T01:25:29Z

🦋 Changeset detected

Latest commit: 09eedb2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
sku	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

askoufis · 2023-08-24T03:51:34Z

packages/sku/context/defaultCompilePackages.js

@@ -14,9 +14,7 @@ try {
  const globs = ['node_modules/@seek/*/package.json'];
  const cwd = skuCwd();

-  const { rootDir, tool } = findRootSync(cwd);
-
-  if (tool === 'pnpm') {


tool is an object, so this was always false.

askoufis · 2023-08-24T03:53:07Z

packages/sku/lib/suggestScript.js

 */
 const findPackageScript = (scriptContents) => {
  let pkg;
  try {
-    pkg = requireFromCwd('package.json');
+    // CWD should be the root of the package
+    pkg = requireFromCwd('./package.json');
  } catch (err) {
    pkg = { scripts: {} };


Resolving package.json from cwd (package root) would always fail, so the catch clause was executing every time, so scripts from package.json were never suggested.

Good catch 🤭

askoufis · 2023-08-24T04:26:04Z

packages/sku/lib/detectYarn.js

-
-module.exports = () => {
-  try {
-    execSync('yarnpkg --version', { stdio: 'ignore' });


yarnpkg is an alias present on debian, but potentially only if the cmdtest package is also installed. Ubuntu (debian) use at SEEK is basically non-existant, so detecting yarn instead of yarnpkg may actually be a bugfix that will make yarn detection work correctly.

Somehow, on my machine this command returns a version number, but in my terminal yarnpkg isn't a valid command.

askoufis · 2023-08-25T01:51:02Z

Tests are failing because multiple storybooks are running simultaneously. There was only one fixture that used storybook, until this PR.

One fixture has a preview.tsx file, which gets symlinked to sku's node modules for storybook to find. There is a cleanup step before the symlinking happens, so the fixtures clash and one cleans up the other one's symlinked file before storybook starts, causing errors.

This issue only happens if the two test suites happen to run at the same time, which seems deterministically based on something, though I don't know exactly what. It would be nice if jest had a way to explicitly disallow a test suite to run in parallel. We could make a separate folder to isolate the test, but that's not great.

I think the best option would be to stop doing all our storybook config shenanigans and just emit files to a .storybook folder (gitignoring main.js only), like we do with other config files, and point storybook there, rather than pointing it to sku's node modules.

This would be beneficial for a few reasons:

Remove the symlinking shenanigans for preview config. Users can just write the file if they need it.
Remove the dummy middleware file. Users can just require/copy/symlink their existing sku dev middleware to .storybook if they want it to run during sku storybook.
There are other config files, namely preview-head.html and preview-body.html that sku doesn't currently support. They allow injecting HTML into the story iframe. These could be useful for webfonts, among other things. Just using the .storybook folder would automatically implement support for these files too.

This concludes my TED talk.

EDIT: Addressed in #878.

askoufis · 2023-08-29T04:54:56Z

Depends on #878.

… comments

packages/sku/package.json

packages/sku/telemetry/provider.js

mrm007 · 2023-09-05T04:06:48Z

packages/sku/lib/suggestScript.js

 */
 const findPackageScript = (scriptContents) => {
  let pkg;
  try {
-    pkg = requireFromCwd('package.json');
+    // CWD should be the root of the package
+    pkg = requireFromCwd('./package.json');
  } catch (err) {
    pkg = { scripts: {} };


Good catch 🤭

mrm007

A few comments related to @antfu/ni, but looks good otherwise 👍

packages/sku/lib/suggestScript.js

packages/sku/lib/validatePeerDeps.js

packages/sku/lib/packageManager.js

.changeset/gorgeous-nails-film.md

Co-authored-by: Remus Mate <3297808+mrm007@users.noreply.github.com>

mrm007

Nice work 👍

mrm007 · 2023-09-06T00:32:04Z

packages/sku/telemetry/provider.js

+const { packageManager, isYarn } = require('../lib/packageManager');
+const { getCommand } = require('@antfu/ni');


I was thinking it would be ideal to absorb @antfu/ni into lib/packageManager so there's only one way to interface with the package manager. But we can iterate on that later.

Good idea. I've got a follow up PR that will use getCommand a bit more, I can refactor it there.

Refactor package manager detection, add support for detecting pnpm

2d92b4d

askoufis requested a review from a team as a code owner August 24, 2023 01:25

Rename constant

4f98186

askoufis force-pushed the detect-pnpm branch from 2441cc0 to 4f98186 Compare August 24, 2023 01:42

askoufis commented Aug 24, 2023

View reviewed changes

Rename isYarn back to useYarn, call yarn instead of yarnpkg

6dbe04d

askoufis force-pushed the detect-pnpm branch from 339a43a to 6dbe04d Compare August 25, 2023 00:16

askoufis mentioned this pull request Aug 29, 2023

Support Storybook config directory #878

Merged

askoufis and others added 6 commits August 31, 2023 10:19

Merge branch 'master' of github.com:seek-oss/sku into detect-pnpm

befe971

Remove some comments

021a4a3

Check for rootDir when detecting default compile packages, add some…

b584986

… comments

Merge branch 'master' of github.com:seek-oss/sku into detect-pnpm

7e16859

Merge branch 'master' of github.com:seek-oss/sku into detect-pnpm

d6de6f9

Merge branch 'master' into detect-pnpm

d6b6e50

mrm007 reviewed Sep 5, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

askoufis and others added 5 commits September 5, 2023 16:23

Upgrade to which@4.0.0

67b3b5c

Use -r why if pnpm

70dbe3d

Update .changeset/gorgeous-nails-film.md

d5b47a7

Co-authored-by: Remus Mate <3297808+mrm007@users.noreply.github.com>

Use @antfu/ni instead of our own API

7fb62ec

Don't need to detect npm if it's our last fallback

9f43a53

askoufis requested a review from mrm007 September 5, 2023 07:07

Invert control flow logic

274bea8

jahredhope approved these changes Sep 5, 2023

View reviewed changes

Update comments about rootDir nullability

09eedb2

mrm007 approved these changes Sep 6, 2023

View reviewed changes

askoufis merged commit 79abdbb into master Sep 6, 2023
4 checks passed

askoufis deleted the detect-pnpm branch September 6, 2023 00:33

seek-oss-ci mentioned this pull request Sep 6, 2023

Version Packages #880

Merged

askoufis mentioned this pull request Sep 6, 2023

Add --packageManager flag, support pnpm in sku init #884

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor package manager detection, add support for detecting `pnpm` #876

Refactor package manager detection, add support for detecting `pnpm` #876

askoufis commented Aug 24, 2023

changeset-bot bot commented Aug 24, 2023 •

edited

askoufis Aug 24, 2023

askoufis Aug 24, 2023

mrm007 Sep 5, 2023

askoufis Aug 24, 2023 •

edited

askoufis Aug 24, 2023

askoufis commented Aug 25, 2023 •

edited

askoufis commented Aug 29, 2023

mrm007 Sep 5, 2023

mrm007 left a comment

This comment was marked as duplicate.

mrm007 left a comment

mrm007 Sep 6, 2023

askoufis Sep 6, 2023

		const { packageManager, isYarn } = require('../lib/packageManager');
		const { getCommand } = require('@antfu/ni');

Refactor package manager detection, add support for detecting pnpm #876

Refactor package manager detection, add support for detecting pnpm #876

Conversation

askoufis commented Aug 24, 2023

changeset-bot bot commented Aug 24, 2023 • edited

🦋 Changeset detected

askoufis Aug 24, 2023

Choose a reason for hiding this comment

askoufis Aug 24, 2023

Choose a reason for hiding this comment

mrm007 Sep 5, 2023

Choose a reason for hiding this comment

askoufis Aug 24, 2023 • edited

Choose a reason for hiding this comment

askoufis Aug 24, 2023

Choose a reason for hiding this comment

askoufis commented Aug 25, 2023 • edited

askoufis commented Aug 29, 2023

mrm007 Sep 5, 2023

Choose a reason for hiding this comment

mrm007 left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

mrm007 left a comment

Choose a reason for hiding this comment

mrm007 Sep 6, 2023

Choose a reason for hiding this comment

askoufis Sep 6, 2023

Choose a reason for hiding this comment

Refactor package manager detection, add support for detecting `pnpm` #876

Refactor package manager detection, add support for detecting `pnpm` #876

changeset-bot bot commented Aug 24, 2023 •

edited

askoufis Aug 24, 2023 •

edited

askoufis commented Aug 25, 2023 •

edited