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

Add Grit migration script #591

Merged
merged 7 commits into from
May 28, 2024
Merged

Conversation

morgante
Copy link
Contributor

You can try this out by running

grit apply valibot_v03

Grit also includes built-in testing for the markdown file. Just run grit patterns test in this repo.

Copy link
Owner

@fabian-hiller fabian-hiller left a comment

Choose a reason for hiding this comment

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

Thank you very much! This looks really nice if it works as expected!


pattern vb_pipe() {
or {
`v.string([$args])` => `v.pipe(v.string(), $args)`,
Copy link
Owner

@fabian-hiller fabian-hiller May 23, 2024

Choose a reason for hiding this comment

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

This should work for any schema function, not just string. You can find a list of all schema functions in our API reference. Also, in the previous version of Valibot, the pipe argument is always the last optional argument of a schema function. Here is another example:

// Before
const Schema = v.map(
  v.number(),
  v.string([v.url(), v.endsWith('@example.com')]), // <-- nested pipe
  [v.maxSize(10)] // <-- outer pipe
);

// After
const Schema = v.pipe(
  v.map(
    v.number(),
    v.pipe(v.string(), v.url(), v.endsWith('@example.com')), // <-- nested pipe
  ),
  v.maxSize(10), // <-- outer pipe
);

But of course there are special cases like union where you can pass an array of options. This is not a pipe. The codemod should just detect an array of actions as the last optional argument of a schema as a "pipe" and refactor only this part. Here is another example:

// Before
const Schema = v.object({
  email: v.string([v.email(), v.endsWith('@gmail.com')]),
  password: v.string([v.minLength(8)]),
  other: v.union([v.string([v.decimal()]), v.number()]), // <--
});

// After
const Schema = v.object({
  email: v.pipe(v.string(), v.email(), v.endsWith('@gmail.com')),
  password: v.pipe(v.string(), v.minLength(8)),
  other: v.union([v.pipe(v.string(), v.decimal()), v.number()]), // <--
});

It is also important that it works with any import wildcard alias as well as with direct imports (same applies to the other codemod steps).

// With "v" alias
import * as v from 'valibot';

// With "foo" alias
import * as foo from 'valibot';

// With direct imports
import { string, minLength,  } from 'valibot';

Copy link
Contributor Author

@morgante morgante May 23, 2024

Choose a reason for hiding this comment

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

Sorry I don't really understand the distinction in the first argument complete - can you take a look at the latest and confirm I interpreted it correctly?

Other cases should work correctly, including non-foo aliases.

To set expectations, no codemod will cover 100% of cases but this should cover 90% of what's required from my reading of the docs - feel free to tweak, I hope the syntax is clear enough.

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you! I will try to have a look later today or in the next few days.

.grit/patterns/valibot_v03.md Outdated Show resolved Hide resolved
@fabian-hiller fabian-hiller self-assigned this May 23, 2024
@fabian-hiller fabian-hiller added the tooling Tooling for devs label May 23, 2024
@fabian-hiller
Copy link
Owner

You can always copy the output to our playground to see if the transformed code is valid.

@fabian-hiller
Copy link
Owner

If I run grit apply valibot_v03 I get the following error: Error: Permission denied (os error 13)

@fabian-hiller
Copy link
Owner

Once I can confirm that the codemod works, I will add instructions on how to run it to our migration guide. Do people need to manually add the .grit/patterns/valibot_v03.md file to their repo to run the command? Does it only works on Linux and MacOS?

@morgante
Copy link
Contributor Author

If I run grit apply valibot_v03 I get the following error: Error: Permission denied (os error 13)

Hmm, what steps did you take to install it / can you provide reproduction instructions? This isn't expected.

Do people need to manually add the .grit/patterns/valibot_v03.md file to their repo to run the command?

Yes, currently they will need to download it. I'm planning to add a feature for applying remote patterns directly too.

Does it only works on Linux and MacOS?

We also have Windows releases available.

@fabian-hiller
Copy link
Owner

Hmm, what steps did you take to install it / can you provide reproduction instructions? This isn't expected.

I followed the Quickstart guide and executed: npm install --location=global @getgrit/launcher. I can run grit. So the installation was successful.

I got it to work with sudo but it should work without it. I will test the codemod in the next few days and get back to you. Thanks again!

Yes, currently they will need to download it. I'm planning to add a getgrit/gritql#356 for applying remote patterns directly too.
We also have Windows releases available.

👍

@morgante
Copy link
Contributor Author

morgante commented May 26, 2024

I got it to work with sudo but it should work without it.

Agreed, sudo definitely shouldn't be required. Maybe you can try running grit doctor without sudo to figure out why. I'm also happy to help debug on Discord.

@fabian-hiller
Copy link
Owner

fabian-hiller commented May 26, 2024

I get the same error when I type grit doctor. With sudo it prints out environment and configuration info. Seems like a permission problem. I will get back to you on Discord tonight or in the next few days. In the meantime, if you have time, you can try to get the following codemod to work.

Hint: transform and brand can be deeply nested and should now be placed next to each other in the pipeline of the wrapped schema.

// Before
const Schema = v.transform(
  v.brand(v.string(), "Name"),
  (input) => input.length
);

// After
const Schema = v.pipe(
  v.string(),
  v.brand('Name'),
  v.transform((input) => input.length)
);

@morgante
Copy link
Contributor Author

This now handles brand and transform.

@fabian-hiller fabian-hiller merged commit b4d1e1e into fabian-hiller:main May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority This has priority tooling Tooling for devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants