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

Implement SkuConfigUpdater for codemodding sku config #1086

Merged
merged 7 commits into from
Nov 21, 2024

Conversation

askoufis
Copy link
Contributor

@askoufis askoufis commented Nov 14, 2024

As part of the upcoming eslint v9 breaking change, the need has arisen to modify consumer's sku config to enable a smoother migration. This PR implements a SkuConfigUpdater class that provides a simple interface for performing imperative updates to a sku config, abstracting away the AST parsing, modification and serialization.

Reviewer notes

AST transformation code can be hard to understand just via visual inspection. The majority of it is just narrowing types to keep the typechecker happy. I wouldn't waste much time reviewing the AST code, the test cases showcase the API much more clearly.

API Example

Below is an example of how the future eslintIgnore option will be inserted into a user's sku config:

// A static method is used here because class constructors can't be async
const updater = await SkuConfigUpdater.fromFile(pathToSkuConfig);

// Modifies the AST of the sku config stored by `updater`
updater.upsertConfig('eslintIgnore', ['**/dist', 'src/types/graphql.ts']);

// Serializes (writes) the AST to disk
await updater.commitConfig();

See the test cases for more examples.

Implementation

SkuConfigUpdater primarily abstracts over magicast, which itself is an abstraction over recast, but uses babel to parse ASTs rather than acorn, and provides a nice Proxy-based API for modifying module imports/exports.

While magicast's API is nice, it doesn't handle CJS exports at all, so I went through a bit of effort implementing logic to deal with sku configs that use module.exports. It's not as clean as I'd like, but I think future effort could be put into automatically uplifting non-TS and/or non-ESM sku configs to TS + ESM, potentially enabling sku to drop support for CJS config entirely.

Dependencies

All dependencies of magicast are already in sku's dependency tree, and magicast itself isn't that large (~120kb), so IMO there's no cause for concern.

Testing

Used this new API to test out fs-fixture, a simple library for creating disposable test fixtures on disk. I think it's pretty neat, and has enabled a relatively simple way to write full integration tests for SkuConfigUpdater solutions that involve mocking the file system.

Misc.

SkuConfigUpdater isn't a great name, definitely open to better suggestions.

@askoufis askoufis requested a review from a team as a code owner November 14, 2024 02:44
Copy link

changeset-bot bot commented Nov 14, 2024

⚠️ No Changeset found

Latest commit: 96255c0

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

packages/sku/lib/SkuConfigUpdater.js Show resolved Hide resolved
packages/sku/lib/SkuConfigUpdater.js Outdated Show resolved Hide resolved
packages/sku/lib/SkuConfigUpdater.js Show resolved Hide resolved
@askoufis askoufis requested a review from a team November 18, 2024 23:39
@askoufis askoufis merged commit b5a3094 into master Nov 21, 2024
4 checks passed
@askoufis askoufis deleted the sku-config-codemod branch November 21, 2024 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants