Open lcnvdl opened 2 months ago
That’s a nice feature! Every time I ran this command, I wished it had kept the prefix! 😁
A few comments on the PR:
- Depending on the package’s dependencies, the
setOrUpdateProjectDependency
method may create or update the dependency. In the case of a creation, the namekeepExistingPrefix
might surprise the reader, as no prefix actually exists. Could it be interesting to improve the name so that it better reflects this? Some ideas:keepExistingPrefixIfAny
,keepExistingPrefixIfExists
,keepExistingPrefixOnUpdate
,keepPrefixIfAny
,keepPrefixIfExists
,keepPrefixOnUpdate
.
I agree with you! Thanks for the feedback!
- Apart from the
upgrade
command, the other CLI commands don't need to keep the prefix of an existing dependency or do anything specific about prefixes. Would it be worth updating thesetOrUpdateProjectDependency
method interface so that only the{ keepExistingPrefix: boolean = false }
option is added? This way, the rest of the CLI code doesn't need to be updated and more straightforward statements likefs.setOrUpdateProjectDependencyOnlyIf(yaml, 'yamljs', '~0.3.0')
increate-app
still work. The prefix would then still be specified with the version number.
That's a good Idea! I'll do it
- To handle the prefix replacement, I wonder if we can do this directly in the
FileSystem
service with more reduced code. For example, the linepkg.devDependencies[name] = version;
could be replaced by something like this:function getPrefix(version: string): string { const firstChar = version[0]; return ['^', '~'].includes(firstChar) ? firstChar : ''; }
Super simple, I like it!
function replacePrefix(version: string, prefix: string): string { const versionWithoutPrefix = version.startsWith(['^', '~']) ? version.substring(1) : version; return
${prefix}${version}
; }
startsWith doesn't work with an array as param, but I understand your idea.
if (pkg.devDependencies[name] && keepExistingPrefix) { const existingPrefix = getPrefix(pkg.devDependencies[name]); pkg.devDependencies[name] = replacePrefix(version, existingPrefix); } else { pkg.devDependencies[name] = version; }
In this way, a layer of abstraction would be removed and the reader would enter the function's behavior more directly, with one click. It would also reduce the amount of code to be maintained in the future. What do you think?
I have to review the code, but I think this leads to code duplication. That's why I've isolated some funcionalities in a different object. But I understand that your priority is to use less amount of files/classes, accepting the cost of having duplicated code blocks.
If that's the case, then I can do a refactor, using your suggestion.
When you create a new FoalTS project, all @foal dependencies begins with the '^' prefix. I think maybe it's a good idea to keep it.
See the NPM docs and semver docs:
Issue
Version prefix is lost after upgrading Foal with the command
foal upgrade
.Solution and steps
Checklist