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.

Sign up for GitHub

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

Jump to bottom

feat(versions): support patch: versions #109

Closed
Aghassi opened this issue Jan 27, 2023 · 7 comments
Closed

feat(versions): support patch: versions #109

Aghassi opened this issue Jan 27, 2023 · 7 comments
Labels
Priority: Low Type: Feat

Comments

@Aghassi
Copy link

Aghassi commented Jan 27, 2023

Description

Currently if you have different packages in a monorepo, and they aren't shared by a workspace, you need to copy paste any patches you create between them. Having a way to enforce that these patches exists and are all the same for any packages that need them would be a helpful check to keeping everything in sync.

Suggested Solution

Add a patches boolean that is defaulted to false, but becomes true in the next major, that let's users opt into this feature. It should respect the grouping that we already do also in the config (if there are multiple versions).

Help Needed
@JamieMason
Copy link
Owner

Can you expand on this please @Aghassi? I don't understand so far, what's a patch in this context?

@Aghassi
Copy link
Author

Aghassi commented Jan 28, 2023

@JamieMason With this package https://www.npmjs.com/package/patch-package you can locally patch NPM packages if you are waiting on upstream changes, or need to modify the logic to support your use case that an upstream maintainer doesn't want to take in. This package adds .patch files (basically a git diff output to a file) into a patches directory in a standard format (including version information). So in a repo where there are multiple packages that use the same patches, I thought it might be useful to have alignment with them. Alternatively, I can just work with the flags patch-package provides to consolidate patches to a single directory and make sure all users point to that directory.

@JamieMason
Copy link
Owner

It looks like this is something officially supported in yarn and pnpm and there's a Yarn example at https://github.com/ryanhs/yarn2-patch-example/blob/master/package.json.

Because they're defined using version numbers, it's in syncpack's remit I suppose – I must admit though I'm not motivated to work on this so I'll leave it in as a low priority to look at after some of the other features have landed. Not many users use patches, but this is quite a cool thing.

@Aghassi
Copy link
Author

Aghassi commented Feb 5, 2023

Yeah I think this is definitely a case of OSS vs Enterprise. We use patches heavily because we may be waiting on an upstream PR and need to move forward while maintaining the fix. Patches allow us to check in the fix without forking the upstream repo. I can hep contribute to this if you can point me to the relevant code spots. But I agree that it's not major, just an edge case I ran into

@JamieMason
Copy link
Owner

JamieMason commented Feb 10, 2023 •
edited
Loading

Do you have any thoughts how you would resolve mismatches in these @Aghassi? In the above example I see superagent": "patch:superagent@6.1.0#patches/superagent.patch" – is that format standardised do you know? or could it vary a lot between projects? I have an idea for how to tackle #112 which I think could sort this as well as a bonus, but if the format of patches is non-deterministic then maybe not.

EDIT: Maybe things like this can be lintable but not fixable, probably good enough for complicated cases.

@Aghassi
Copy link
Author

Aghassi commented Feb 11, 2023

I haven't yet tried with pnpm but the API surface between yarn patch and pnpm patch look very similar if not identical (https://pnpm.io/cli/patch-commit & https://pnpm.io/cli/patch-commit).

I believe there are two scenarios:

  1. You don't have a workspace and therefore you can't have two versions resolved. If you find two keys for the same package, probably best to error out and ask the user to flatten the dep graph.
  2. You don't have workspaces, but you are in a repo with multiple package.json. You could have different versions here and if you want alignment that's up to the repo owner. This case is hard and I don't have an opinion on how best to handle it.
  3. You have workspaces which means you can have nested package.json each of which could have a different patch version. In this case, we probably want to still align on the version of the dependency as the patch file doesn't have the version information in it (or shouldn't?). I'd say it is also best to add a config option to disable this feature or gate it at first so that you can weed out edge cases.

I am biased towards 3. because that's my scenario and I don't want multiple versions of anything unless I am using the versionGroups feature, in which case I would guess that the patch linting behaves the same. In general, I want one version.

I'm not sure (yet) if the syntax is standard. I'm migrating to pnpm now internally so I will have a better sense as I migrate. I agree lintable but not fixable is a good start, as well as bias towards erroring not warning.

Thoughts?

@JamieMason JamieMason changed the title Support aligning patches files when using patch-package feat(versions): support patch: versions May 28, 2023
JamieMason added a commit that referenced this issue May 28, 2023
@JamieMason
refactor(core): rewrite core architecture
1fea11b 
Closes #124

When using the `workspace` dependency type, packages installing that dependency
no longer have to exactly match the `version` property of the package.json of
origin.

If the version or version range used by every dependent package matches, it is
considered valid.

Closes #130

JavaScript config files now have support for TypeScript IntelliSense.

https://jamiemason.github.io/syncpack/config-file#typescript-intellisense

Closes #114
Refs #109
Refs #125

Unsupported versions can now at least be managed via `pinVersion`, where
previously anything which was not valid semver would be ignored.

Refs #111
Refs #132

TypeScript IntelliSense support helps catch invalid config, but more work is
needed to display useful error messages at runtime.

Refs #48
Refs #3

Syncpack's internals are now better organised, so providing a Node.js API and
general lint and fix CLI commands are now closer to being released.

BREAKING CHANGES:

Although they are still not auto-fixable, unsupported versions which were
previously ignored are now acknowledged, which may introduce mismatches which
previously would have been considered valid.

This release was also a huge rewrite of Syncpack's internals and, while there
is a large amount of tests, some scenarios may have been missed.

If you run into any problems, please create an issue.
JamieMason added a commit that referenced this issue May 28, 2023
@JamieMason
refactor(core): rewrite core architecture
6ca61cc 
Closes #124

When using the `workspace` dependency type, packages installing that dependency no longer have to exactly match the `version` property of the package.json of origin.

If the version or version range used by every dependent package matches, it is considered valid.

Closes #130
Closes #131

JavaScript config files now have support for TypeScript IntelliSense.

https://jamiemason.github.io/syncpack/config-file#typescript-intellisense

Closes #114
Refs #109
Refs #125

Unsupported versions can now at least be managed via `pinVersion`, where previously anything which was not valid semver would be ignored.

Refs #111
Refs #132

TypeScript IntelliSense support helps catch invalid config, but more work is needed to display useful error messages at runtime.

Refs #48
Refs #3

Syncpack's internals are now better organised, so providing a Node.js API and general lint and fix CLI commands are now closer to being released.

BREAKING CHANGES:

- `fix-mismatches` will now exit with a status code of 1 if there are mismatches among unsupported versions which syncpack cannot auto-fix.
- Although they are still not auto-fixable, unsupported versions which were previously ignored are now acknowledged, which may introduce mismatches which previously would have been considered valid.
- This release was also a huge rewrite of Syncpack's internals and, while there is a large amount of tests, some scenarios may have been missed.
- If you run into any problems, please create an issue.
JamieMason added a commit that referenced this issue May 28, 2023
@JamieMason
fix(core): rewrite core architecture
dc9355f 
### #124

When using the `workspace` dependency type, packages installing that dependency no longer have to exactly match the `version` property of the package.json of origin.

Closes #124

If the version or version range used by every dependent package matches, it is considered valid.

### #130, #131

JavaScript config files now have support for TypeScript IntelliSense.

Closes #130
Closes #131

https://jamiemason.github.io/syncpack/config-file#typescript-intellisense

### #109, #114, #125

Unsupported versions can now at least be managed via `pinVersion`, where previously anything which was not valid semver would be ignored.

Closes #114

### #111, #132

TypeScript IntelliSense support helps catch invalid config, but more work is needed to display useful error messages at runtime.

### #48, #3

Syncpack's internals are now better organised, so providing a Node.js API and general lint and fix CLI commands are now closer to being released.

BREAKING CHANGE:

- `fix-mismatches` will now exit with a status code of 1 if there are mismatches among unsupported versions which syncpack cannot auto-fix.
- Although they are still not auto-fixable, unsupported versions which were previously ignored are now acknowledged, which may introduce mismatches which previously would have been considered valid.
- This release was also a huge rewrite of Syncpack's internals and, while there is a large amount of tests, some scenarios may have been missed.
- If you run into any problems, please create an issue.
@JamieMason
Copy link
Owner

Released in syncpack@10.0.0, please let me know if you run into any issues. patch: packages will no longer be ignored, if they are identical they will be valid, if they mismatch they will be reported but they are not auto-fixable.

Note that for nested workspaces there is also #133 which is not yet complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Priority: Low Type: Feat
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@JamieMason @Aghassi