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

Versioning for nested packages #491

Closed
3 tasks done
dlueth opened this issue Feb 26, 2023 · 37 comments
Closed
3 tasks done

Versioning for nested packages #491

dlueth opened this issue Feb 26, 2023 · 37 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@dlueth
Copy link

dlueth commented Feb 26, 2023

Clear and concise description of the problem

As a developer using Lerna-Lite I would love to have the possibility to work with nested packages in a way that changes to a package B being a subpackage of package A directory-wise do not lead to a version-change of package A on lerna version purely due to git diff but only if package A actaully depends on package B. This example reflects our current development structure but produces many new, but unnecessary, versions.

Suggested solution

Potential fix would be in packages/core/src/utils/collect-updates/lib/make-diff-predicate.ts. Replace the if-condition starting on line 65 with the following code:

if (formattedLocation) {
    // avoid same-directory path.relative() === ""
    const excludeSubpackages = execSync("find", [formattedLocation, "-mindepth", "2", "-type", "f", "-name", "package.json" ])
      .split("\n")
      .filter((file) => !!file)
      .map((file) => `:^${path.dirname(file)}`);

    // avoid same-directory path.relative() === ""
    args.push('--', formattedLocation, ...excludeSubpackages);
  }

Alternative

It would also be possible to exclude subpackages after getting the diff via packages like read-pkg-up but that feels suboptimal due to the overhead for diffing and necessary filesystem operations implied.

Additional context

No response

Validations

  • Follow our Code of Conduct
  • Read the docs.
  • Check that there isn't already an issue that request the same feature to avoid creating a duplicate.
@ghiscoding
Copy link
Member

That's a bit outside of my comfortable zone and knowledge of the code, feel free to create a Pull Request with this change, however this seems like a potential change of behavior and it would probably be better to put this under a new flag to not break expectation.

@dlueth
Copy link
Author

dlueth commented Feb 27, 2023

@ghiscoding Yea, I do understand. Already thought about possible negative effects for others but could not find a problematic case, but that's just what I had in mind. But you do get what the change aims at, do you?

Just to give a bit more background: We are maintaining a > 600 packages Monorepo with some packages having something like 20 subpackages and other packages with > 60 dependencies from within the repo. So any unnecessary version can easily lead to a 100 new versions on packages that would really need no change at all.

I will see if I am able to deal with implementing a flag for this to be enabled when required and issue a PR ;)

@ghiscoding ghiscoding added enhancement New feature or request help wanted Extra attention is needed labels Feb 27, 2023
@ghiscoding
Copy link
Member

ghiscoding commented Feb 27, 2023

wow that is quite a large monorepo, my biggest one is still below 20 packages and I use the same version for all packages (not independent). I assume that you are using the independent mode which is why you want less changes detected (probably make sense with such a large mono). So anyway, I would still prefer a flag to avoid possible surprises for some, though I know this would involve more code change, it's a safer approach to avoid surprises

@ghiscoding
Copy link
Member

ghiscoding commented Mar 1, 2023

Already thought about possible negative effects for others but could not find a problematic case, but that's just what I had in mind. But you do get what the change aims at, do you?

@dlueth so I copied your code and gave it a try and wanted to see if any unit tests failed and we sure do have some failures in regards to that predicate file (because we have 2 execSync instead of 1 with mocked returned values). That's fine, we can fix that, but then after looking at the new command you added (which I have never used before), I just realized that it's a Unix only command so for a person like me, on Windows only, that would definitely be a breaking change. So we definitely need a flag. Are there any possibilities to have a cross-browser command, perhaps with Glob? Also what does this find command supposed to return? I need to know what the mock values that this is suppose to return for all the unit tests.

@dlueth
Copy link
Author

dlueth commented Mar 1, 2023

@ghiscoding Ah, you are right. Find is Unix/Linux/BSD/MacOS only. Have not worked with Windows for a looooong time so completely forgot that.

What the find command does is it looks for package.json files (-type f) any depth below (but not in) the current directory (-mindepth 2). So you effectively get a list of all subpackages of the current one and simple have to remove the filename to get their basedir which is than added to git diff to be excluded.

Although this is most likely the "fastest" way it is not the only way - and, as you mentioned, not working on Windows :(

2 Options:

  1. it should be relatively easy to replace the execSync find with NPM package globby (which would probably be the fastest way)
  2. one could filter the changedFiles between line 44 & 46 of the same file and utilize something like load-pkg to find the nearest package.json for every changed file and check if the package-name it includes is the one of the package currently processed

@dlueth
Copy link
Author

dlueth commented Mar 1, 2023

@ghiscoding I will prepare a glob snippet for you, seems to better fit than globby

@dlueth
Copy link
Author

dlueth commented Mar 1, 2023

OK, as globby is already present I implemented it utilizing it. You need to add import globby, { GlobbyOptions } from 'globby';

In addition, replace the complete const excludeSubpackages assignment with:

const excludeSubpackages = globby.sync('**/*/package.json', { cwd: formattedLocation, nodir: true, ignore: '**/node_modules/**' } as GlobbyOptions)
      .map((file) => `:^${formattedLocation}/${path.dirname(file)}`);

@ghiscoding
Copy link
Member

ghiscoding commented Mar 2, 2023

@dlueth I created a PR #495 with the information you provided, could you please review it. I'm also not sure if the flag name is a good name (it could be changed), let me know if I misunderstood anything or if the docs I wrote is incorrect or is missing something.

The new opt-in option is the following

lerna version --exclude-subpackages

@dlueth
Copy link
Author

dlueth commented Mar 2, 2023

@ghiscoding Thanks, a lot of work you had to do! I reviewed as far as I could and commented where necessary. I think --exclude-subpackages shoudl better be --independent-subpackages because the relation change is in fact the other way round.

@ghiscoding
Copy link
Member

@dlueth thanks for the review, I'll rename it, however I think you might have forgot to finish the review because I don't see any comments in the PR

@dlueth
Copy link
Author

dlueth commented Mar 2, 2023

@ghiscoding Yep, sorry - forgot to submit the review... Still a bit lazy at 6:12 in the morning XD

@ghiscoding
Copy link
Member

and it's time for me to go to bed, it's past midnight here in Eastern Canada 😉
Thanks for providing the code, there's still a lot of Lerna's code that I'm unfamiliar with

@dlueth
Copy link
Author

dlueth commented Mar 2, 2023

@ghiscoding hope it's OK to reuse this issue instead of opening a new one for one related question left:

Do you happen to know if it is already possible to pass in/through excludeDependents to packages/core/src/utils/collect-updates/collect-updates.ts => collectUpdates function?

If so that would be absolutely awesome. If not, would it be possible make this a public flag as well?

@ghiscoding
Copy link
Member

@dlueth
It already seem to be used in the collectUpdates method which is then used by collectPackages

image

in collectUpdate we see

return collectPackages(packages, {
onInclude: (name) => log.verbose('updated', name),
excludeDependents,
});
}

and

return collectPackages(packages, {
isCandidate: (node, name) => isForced(node, name) || needsBump(node) || hasDiff(node),
onInclude: (name) => log.verbose('updated', name),
excludeDependents,
});

then used in collectPackages

if (!excludeDependents) {
collectDependents(candidates).forEach((node) => candidates.add(node));
}

So after looking at all that code, which came from Lerna, it does seem to be implemented. Have you tried it? Should I wait to prepare a release?

@dlueth
Copy link
Author

dlueth commented Mar 2, 2023

@ghiscoding nö need to wait with a release for this. But yes, I saw it implemented there but is it settable via a Lerna version CLI flag?

@ghiscoding
Copy link
Member

@dlueth
the docs is available on the filter-packages readme page, take a look and see if that answers your question

@dlueth
Copy link
Author

dlueth commented Mar 2, 2023

@ghiscoding so I need to install the optional filter package and I should be able to use this flag with lerna version? I wonder because docs nowhere state it can be used with version command

@ghiscoding
Copy link
Member

no it's a subpackage used by the core, it's installed by default and there's nothing for you to do to take advantage of it. I simply sent you the link to provide you the docs, which seems to be the only docs available, I believe it is global and can be used by any Lerna command (version, publish, run, ...).

@dlueth
Copy link
Author

dlueth commented Mar 2, 2023

@ghiscoding thx, will check that tomorrow and report back. That would be perfect for our usecase ;)

@ghiscoding
Copy link
Member

actually I might be wrong, it looks like it's only restricted to certain commands, so I don't think that would work for lerna version, we need to try to see

image

@ghiscoding
Copy link
Member

ghiscoding commented Mar 3, 2023

@dlueth so this might not work because like I found in the post just before it wasn't part of the version & publish version, but just the sake of trying it out, I added --exclude-dependents to the lerna version CLI in PR #505. I would totally suggest you to give it a try with the --dry-run option just to see what it does, I added dry-run for troubleshooting so it's helpful in that case. For a quick test, I added a console log in the collectUpdate and it does log true, so it is enabled but again I'm not sure if it will work since it wasn't intended to work with version & publish... anyway, give it a try and see what happens

@dlueth
Copy link
Author

dlueth commented Mar 3, 2023

@ghiscoding Release with the new flag works as perfectly fine & as expected. Regarding the --exclude-dependents:

I can confirm the CLI flag reaches the code but it has some quirks when used with lerna version. It seems as if it not only leads to no bump on the dependent package but does also not update the package.lock of it. So lerna version will afterwards fail when running npm install --package-lock-only. That might be fixable by utilizing workspace-protocol instead of fixed version numbers I presume. Will give it a try later

@ghiscoding
Copy link
Member

not surprising that there's quirks around the usage of that flag since it wasn't specifically added to the lerna version command.

If you're not using workspace: protocol (yarn/pnpm, not available for npm though), you can look at using --sync-workspace-lock which is an option I added to sync up the lock file by using the package manager to update the lock file, the main advantage is that since it's part of the version/publish process, it will also be committed. If on the other hand, you're using workspace: protocol then the --sync-workspace-lock is probably redundant

Also just as a reminder, I wrote it on the main readme but in case you didn't see... my main purpose when creating Lerna-Lite fork was to update all deps (when Lerna was no longer supported a year ago), make it more modular and add couple of features like the workspace: protocol and these tasks were all achieved. However there's still a lot of the internal code that I just don't understand or simply not familiar enough to take as a task (for example I'm a Windows person, never use Unix & Git commands are mostly googled when I need them). I'm mentioning this because adding exclude-dependents might be one of these things that I'm not sure that I have enough knowledge to proceed with a change, so at least now you know. Just trying my best 😉

@dlueth
Copy link
Author

dlueth commented Mar 3, 2023

@ghiscoding many thanks for all your efforts! In our case it seems to be sufficient to simply use "*" instead of a version for NPM. We do not need to publish to a registry luckily ;)

@ghiscoding
Copy link
Member

@dlueth did you end up using the --exclude-dependents with lerna version? If not then I'll remove it from the version CLI commands (rolling back PR #505).

@dlueth
Copy link
Author

dlueth commented Mar 3, 2023

@ghiscoding I did, yes. No need to roll it back but I am not 100% it works as it should in this case. Still testing

@dlueth
Copy link
Author

dlueth commented Mar 3, 2023

@ghiscoding OK, I think I have finished my testing but results are highly specific for our Project/Monorepo:

  1. This is a very large Monorepo with currently about 400 packages that will most likely end up having >600 packages
  2. The Monorepo is a complete Website (so "Frontend") + Middleware + Cloudflare-Worker partially sharing packages and is built via Gitlab as a whole...
  3. ... so we do NOT need to publish any packages to any kind of package registry
  4. The whole repo ist based on NPM workspaces

The trick I used so far, as NPM does not support workspace:*, is to simply put * into any internal depency versions. We can do this because we do NOT need to ever publish any package.

We are currently running lerna version with --no-push --sync-workspace-lock --independent-subpackages --exclude-dependents.

What happens now is that lerna version replaces all * for the internal packages with their actual versions and, within the process, runs npm install --package-lock-only. So far so good and this works as long as there are only * versions in any package.lock when starting lerna version...

... but it fails hard whenever you do a new release afterwards and there are any other versions than * in any package.json.

So i wrote a post-script that resets all versions to * which works as we do use --no-push and amends the changes to the previous commit. In addition it needs to fetch all tags from lerna version's commit, delete them and append them to the new comit. Seems to work so far, but it would be really nice to get rid of the extra post-script.

@ghiscoding
Copy link
Member

ghiscoding commented Mar 3, 2023

The trick I used so far, as NPM does not support workspace:*, is to simply put * into any internal depency versions. We can do this because we do NOT need to ever publish any package.

Whenever I go with multiple, I now always choose pnpm and workspace: protocol, the main difference is that it always use local code (it will never try to fetch from the registry) as oppose to using "*" with NPM which will try to fetch from the registry and only after use local code.

I don't remember or know how Lerna deals with *, what I can tell you is how workspace: is dealt with in the project since I did implement that portion. It's first used by lerna version with the line below. This will mutate/replace the workspace:* with the actual version number like "2.0.0" when calling this method (basically the updateLocalDependency() is the method that mutates workspace:* to a version number back and forth). Lerna might be doing something similar when is sees file: or link:

if (depVersion && resolved.type !== 'directory') {
// don't overwrite local file: specifiers, they only change during publish
pkg.updateLocalDependency(
resolved,
depVersion,
this.savePrefix,
this.options.allowPeerDependenciesUpdate,
this.options.workspaceStrictMatch,
this.commandName
);
}
}

in this process it also pushes git changes and creates git tags, then when the user is ready to publish it goes and call the same method to mutate back the package to its original version value workspace:* via this call

resolveLocalDependencyWorkspaceProtocols() {
// resolve workspace protocol: translates to their actual version target/range
const publishingPackagesWithLocalWorkspaces = this.updates.filter((node: PackageGraphNode) =>
Array.from(node.localDependencies.values()).some((resolved: NpaResolveResult) => resolved.workspaceSpec)
);
return pMap(publishingPackagesWithLocalWorkspaces, (node: PackageGraphNode) => {
// regardless of where the version comes from, we can't publish 'workspace:*' specs, it has to be transformed for both local & external dependencies
// e.g. considering version is `1.2.3` and we have `workspace:*` it will be converted to "^1.2.3" or to "1.2.3" with strict match range enabled
// 1. update & bump version of local dependencies
for (const [depName, resolved] of node.localDependencies) {
const depVersion = this.updatesVersions?.get(depName) || this.packageGraph?.get(depName)!.pkg.version;
// it no longer matters if we mutate the shared Package instance
node.pkg.updateLocalDependency(
resolved,
depVersion,
this.savePrefix,
this.options.allowPeerDependenciesUpdate,
this.options.workspaceStrictMatch,
this.commandName
);
}
// 2. remove any "workspace:" prefix from the package to be published any of external dependencies (without anything being bumped)
// we will only accept "workspace:" with semver version, for example "workspace:1.2.3" is ok but "workspace:*" will log an error
for (const [_depName, resolved] of node.externalDependencies) {
node.pkg.removeDependencyWorkspaceProtocolPrefix(node.name, resolved);
}
// writing changes to disk handled in serializeChanges()
});
}

I'm not sure if similar mutations are done with *, possibly, but what I believe is that since you aren't calling a publish then it might why it's not mutating back to its original value workspace:*, I'm actually not 100% on this but we can easily validate that by calling lerna version --dry-run and see what are showing up in any of the package.json (I'm guessing it's the version number, not workspace:* anymore) and if that is the case then the fact that you're not calling lerna publish might be why it's never mutating back to its original value workspace:*... at the end of the day, Lerna internally does something similar to what you're doing which is to mutate the version from/to workspace:* and a version number, the only difference is that in some cases it is done in memory rather than writing directly to the file (for example, I think it changes the version number in memory when creating the tarball and simply disregards what is in memory after it's published). You can use dry-run on both version and publish to see what happens without versioning or publishing anything, so it would be good to test this out, I also added the dependencies on the publish dry-run to inspect what was being published, it helped me troubleshoot some errors I had when implementing the feature.

I most say, I don't fully understand why you're calling lerna version if you're not publishing? It would be helpful to see your expected workflow. Are you using it just to create git tags of version at that point in time, or is it for other reasons? Have you also searched on the original Lerna issues, I often search there to see if anyone posted anything similar with possible solutions.

@dlueth
Copy link
Author

dlueth commented Mar 3, 2023

@ghiscoding I would say that our need is really special but I thought, and you seem to agree, that Lerna seems to be doing something similar under the hood ;)

We do not need to publish because the complete project is the monorepo. There is absolutely no need to ever use any of it's packages from somewhere else. But, what we need is the relationship between packages and, even more important, versions.

We do write ES6 packages with static imports but transform them, via Rollup, to a format that is a bit similar to AMD. So what is written as a static import becomes a dynamic dependency (if from the monorepo) and gets picked up by a loader in the frontend that loads it with its version and caches it in local storage for fastest reuse as possible. To further speed up the, in an AMD context, chain of step by step package resolution and dependency loading we have a lookup JSON included that contains every package, its version and all it's dependencies, recursively.

Just to share the rough idea XD

@ghiscoding
Copy link
Member

ghiscoding commented Mar 3, 2023

I still wish to see the workflow of the expected steps (1. bump version, 2. git tag, ....).

Looking at Lerna's issue, I found these 2 comments which might be helpful (or not).

@dlueth
Copy link
Author

dlueth commented Mar 4, 2023

  1. Find only packages that did actually change by themselves
  2. Bump their version according to relevant commit messages, in their own package.json only
  3. Update root package-lock.json
  4. Commit changes
  5. Add version tags to the commit

Looking at the comments you linked I would presume that my current status is as close to these steps as we can get. Adding no git-tags via Lerna would mean we would have to re-implement it's logic of what actually changed. So it seems easier to read out and re-apply the tags Lerna added currently

@ghiscoding
Copy link
Member

After trying with dry-run, it looks like I was wrong in the sense that it doesn't replace any dependencies with workspace:*, it only bumps the package "version" property (which seems to be what you want). It will only do the workspace:* replacement when creating the tarball and publishing (which you aren't doing), so I guess you should be fine but maybe only when using workspace:*, I don't know what happens when you use only *, it might be doing something totally different. Any chances you can try with pnpm or yarn workspace: protocol? It looks like you might be more successful than with just *. If you want to test workspace: and see if that would work but don't want to test on your large monorepo, you could perhaps try it on Lerna-Lite which I saw you forked already, so maybe try it there and see if that would work (Lerna-Lite is using pnpm workspace:*)

For the steps that you provided, which ones are working and which ones are you having problems with?
Cheers

@dlueth
Copy link
Author

dlueth commented Mar 5, 2023

If you use * (with NPM that is) it replaces it with actual versions on lerna version. I tried Yarn briefly but it, in general, seems to be having problems resolving our packages at all (regarding workspaces and package paths) as both are deeply nested on the workspace as well as package name filesystem structure.

From the bullets above only 2. is actually not working for our case. The rest works just fine

@ghiscoding
Copy link
Member

I'm afraid there isn't much more I can do at this point, the implementation of * in Lerna's code have been in place for many years so I'm expecting it to be correct.

Personally with the issue you're having, I would give a try to pnpm workspace:*, I'm pretty sure you would have more luck with pnpm rather than npm and even yarn. The workspace: won't get replaced by an actual version since it only replaces when publishing and it's done in memory, I've checked that couple days ago. You also get a lot more support when using pnpm, they are pretty responsive and would fix any issues related to workspace if you have any. There's maybe a last thing you can try, but I'm not sure how successful that would be, which is to use file: or link: protocol

@dlueth
Copy link
Author

dlueth commented Mar 6, 2023

@ghiscoding yeah, I think we will try out pnpm when we find some time as our needs are clearly a bit special and most likely out of scope regarding Lerna+ NPM.

Thanks a thousand times for all your (awesome) efforts for such weird requirements ;)

@dlueth
Copy link
Author

dlueth commented Mar 15, 2023

@ghiscoding I ended up removing Lerna completely. I refactored most of our scripts to run without Lerna over the last 2 weeks and the only thing left we needed was Lerna version anyway. Having had the base from refactoring the other scripts a minimal, but sufficient, replacement for our case took approx half a day extra

@ghiscoding
Copy link
Member

oh ok, that's too bad you're not using anymore but if it simplifies your process then it's probably best for you.. and you can always use it on future project 😉 Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants