statelyai / xstate

Actor-based state management & orchestration for complex app logic.
https://stately.ai/docs
MIT License
27.23k stars 1.26k forks source link

Clean up files, exports, dependencies #4974

Closed with-heart closed 3 months ago

with-heart commented 4 months ago

This PR installs and configures knip which I then used to clean up a number of things in the repo:

I think it'd be cool to eventually integrate knip into our pipeline to be able to automate catching these things but for now doing going through them manually is a good start.

P.S.: Since stuff is changing all over the place, it might be easier to read the changes commit-by-commit

P.P.S.: Press n to go to the next commit and press p to go to the previous commit

changeset-bot[bot] commented 4 months ago

⚠️ No Changeset found

Latest commit: f233b2fb96059a77b8d1935d7ae39e88f1697756

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

with-heart commented 4 months ago

The workspaces config I used for examples and templates was incorrect (examples instead of examples/*). Actually enabling them creates a bunch of extra work like needing to install all of their dependencies so I just removed it and will revisit later maybe

webpro commented 4 months ago

Linked from https://github.com/webpro-nl/knip/issues/735, thanks for trying Knip! I saw the config and thought I might be able to help out a little.

From running Knip in this repo, I was able to fix two bugs, thanks! Re. babel overrides and old husky.

Knip automatically picks up package.json#workspaces, but looks like we can use scripts/*.js for the root and then the rest seems to work out fine.

Now we can simplify the config a bit:

{
  "$schema": "https://unpkg.com/knip@5/schema.json",
  "workspaces": {
    ".": {
      "entry": ["scripts/*.js"],
      "project": ["scripts/*.js"]
    }
  },
  "ignore": [
    // used for `#is-development` conditional import
    "packages/**/{true,false}.ts",
    // file acts as a type test
    "packages/xstate-svelte/test/interpreterAsReadable.svelte"
  ],
  "ignoreBinaries": ["svelte-check", "docs:build"],
  "ignoreDependencies": [
    "@xstate-repo/jest-utils",
    "@xstate-repo/jest-utils/setup",
    "synckit",
    // package.json#exports aren't added as entry points, because `dist/` is .gitignored
    "react",
    "xstate",
    "@types/ws"
  ]
}

There's remaining output:

❯ knip
Unused devDependencies (2)
@tsconfig/svelte  packages/xstate-svelte/package.json
svelte-jester     packages/xstate-svelte/package.json
Unlisted dependencies (1)
svelte-jester  jest.config.js

You could try now using yarn add -D https://pkg.pr.new/knip@bd16c98 or wait a little for the next GA.

AMA :)

Andarist commented 4 months ago

@tsconfig/svelte is confusing because of packages/xstate-svelte/test/package.json - maybe just add to ignoreDependencies

I assume this might be some issue with pkg.json lookup. Such "package.json scopes" to add type are... weird but I've seen them used them in the wild a couple of times already. Maybe it would be worth ignoring them in Knip? This is a documented pattern in the node.js docs.

with-heart commented 4 months ago

@webpro Thanks so much for taking a look! Glad you were able to fix two bugs while helping us clean up the config. And also glad to learn about pkg.pr.new, that's quite a useful tool.

I was thinking about posting on twitter to ask if I knew any knip experts that wanted to take a look at the config but opening an issue got us the expert. The power of open source ❤️

with-heart commented 4 months ago

I believe this is good to go now 😁

webpro commented 4 months ago

@tsconfig/svelte is confusing because of packages/xstate-svelte/test/package.json - maybe just add to ignoreDependencies

I assume this might be some issue with pkg.json lookup. Such "package.json scopes" to add type are... weird but I've seen them used them in the wild a couple of times already. Maybe it would be worth ignoring them in Knip? This is a documented pattern in the node.js docs.

Sorry, not sure exactly what you are referring to here. Knip follows the root package.json#workspaces to find workspaces (extra workspaces can be configured, though). If we ignore packages/xstate-svelte/test/package.json in this regard, then the easiest path is to configure test/tsconfig.json as the path of the TypeScript config for this workspace:

{
  "workspaces": {
    "packages/xstate-svelte": {
      "typescript": "test/tsconfig.json",
      // shorthand for "typescript": { "config": ["test/tsconfig.json"] }
    }
  }
}
with-heart commented 4 months ago

Since knip@5.27.0 was released this morning, I've replaced pkg.pr.new with ^5.27.0. Running it results in no errors so we should be good to go! 🤞

webpro commented 4 months ago

Thanks @with-heart! ➕❤️

You could also use the config from my latest comment to get rid of of the ignored @tsconfig/svelte issue, but it's minor.

Andarist commented 4 months ago

I guess I just don't understand why this has to be declared as an extra workspace in the first place - couldn't it be picked up automatically? It's fine as a solution right now - I just wonder how Knip's magic sauce is made and if there is any room for improvements on that front

webpro commented 4 months ago

I guess I just don't understand why this has to be declared as an extra workspace in the first place - couldn't it be picked up automatically?

There's no magic here, Knip just uses package.json#workspaces - which does not include packages/xstate-svelte/test.

Andarist commented 4 months ago

Right, because it's not a workspace. But including it in Knip's workspaces it feels like we dilute the meaning of a workspace. Multiple tsconfig.jsons (and package.jsons for that matter) are fair game within a single workspace. So I just wonder if they couldn't be discovered automatically? It would require less work for the user.

webpro commented 4 months ago

Right, because it's not a workspace. But including it in Knip's workspaces it feels like we dilute the meaning of a workspace.

We're not including it as a workspace, it's only Knip's TypeScript plugin that pulls in test/tsconfig.json instead of ./tsconfig.json so it can extract dependencies from that file. There's nothing more to it, it's not like we're defining ./test as a TS project or a (npm/Yarn) workspace.

Multiple tsconfig.jsons (and package.jsons for that matter) are fair game within a single workspace. So I just wonder if they couldn't be discovered automatically? It would require less work for the user.

Unfortunately we're simply not there yet.

davidkpiano commented 4 months ago

Is this one good to go on (pending merge conflicts)? @Andarist @with-heart

with-heart commented 4 months ago

Yeap we're good to go!