-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: ignore packages via release-config #55
base: master
Are you sure you want to change the base?
Conversation
lib/getPackagePaths.js
Outdated
// handle ignorePackages in config | ||
const msrOptions = getConfigMsrSync(cwd); | ||
if (msrOptions.hasOwnProperty("ignorePackages")) { | ||
if (Array.isArray(msrOptions.ignorePackages)) ignorePackages.push(...msrOptions.ignorePackages); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (Array.isArray(msrOptions.ignorePackages)) {}
No need to check field presence. isArray
would be enough.
And... I think msr config value should be used as fallback only.
if (ignorePackages === null && Array.isArray(msrOptions.ignorePackages)) {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (Array.isArray(msrOptions.ignorePackages)) {}No need to check field presence.
isArray
would be enough.
good catch. just pushed a fix for that.
And... I think msr config value should be used as fallback only.
@antongolub i don't agree.
i would find it confusing that once i provide an option via commandline the settings in my config were to be ignored.
the cli-ignores already get applied on top of workspace-ignores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davikawasaki, what do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@betaboon I'm concerned about how confusing this is in terms of precedence. I agree with @antongolub that MSR config should be used as fallback. Here's my train of thought on this:
Here's how I'd do this logic:
- First the script checks if CLI sets ignored packages (accepted packages are all except the ignored ones)
- If CLI doesn't ignore packages, check ignored and accepted packages from yarn/pnpm/bolt workspaces
- If none of the configuration is set above, get the msr ignored and accepted packages
- If it still is null, set all packages as accepted for deployment
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh honest to me that approach is more confusing :P
examples
workspace-settings only:
- the workspace has packages
a
,b
,c
,d
- the workspace-setting (eg in
package.json
) ignores packaged
(i.e.!d
) - this results in packages to be released:
a
,b
,c
workspace-settings + cli:
- the workspace has packages
a
,b
,c
,d
- the workspace-setting (eg in
package.json
) ignores packaged
(i.e.!d
) - the cli-options provide package
c
to be ignored - this results in packages to be released:
a
,b
workspace-settings + msr-config + cli (my suggestion):
- the workspace has packages
a
,b
,c
,d
- the workspace-setting (eg in
package.json
) ignores packaged
(i.e.!d
) - the msr-config ignores package
c
- the cli-options provide package
b
to be ignored - this results in packages to be released:
a
workspace-settings + msr-config + cli (your suggestion?):
- the workspace has packages
a
,b
,c
,d
- the workspace-setting (eg in
package.json
) ignores packaged
(i.e.!d
) - the msr-config ignores package
c
- the cli-options provide package
b
to be ignored - this results in packages to be released:
a
,c
,d
reasoning
so if i understand our argument correctly we are arguing about two approaches:
- add-up (workspace-settings + msr-config + cli-options)
- most-specific (priority: workspace-settings < msr-config < cli-options)
why i would argue against most-specific
:
imagine the the project looking like this:
├── apps
│ └── example
├── package.json
└── packages
├── private
│ ├── a
│ ├── b
│ └── c
└── public
├── d
├── e
└── f
and the package.json
contains:
"workspaces": [
"!private/**",
"public/**",
"apps/**"
]
now running msr in the build-pipeline with --ignored-packages apps/example
would effectively override the settings in package.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a monorepo world, having a package that's not part of the workspace would make sense? Wouldn't it be treated as a isolated package? Because this could show that even though this package isn't part of the workspace, we could still release it.
Following this train of thought, I'd be willing to consider the ignore-package
feature only through the CLI and the msr releaserc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm-documentation has a good example why you would want to exclude packages from the workspace: https://pnpm.js.org/pnpm-workspace_yaml (# exclude packages that are inside test directories
)
In a monorepo world, having a package that's not part of the workspace would make sense? Wouldn't it be treated as a isolated package? Because this could show that even though this package isn't part of the workspace, we could still release it.
iirc @manypkg/get-packages
doesn't even list the packages that are ignored in the workspace-settings, thus they will be implicitly ignored when publishing, which i would consider the "correct" solution.
Following this train of thought, I'd be willing to consider the
ignore-package
feature only through the CLI and the msr releaserc.
so we're down to override vs extend
on .releaserc < cli
vs .releaserc + cli
.
I'd vote for extend
but in the end it's up to you as the maintainers and i am fine with any of those solutions and will rework the PR accordingly :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antongolub @davikawasaki what can we do to proceed on this issue ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msr is just a wrapper over the semantic-release, so I think we should inherit its CLI mechanics: CLI flags shoould override config options. In this case, we will have simpler, consistent and more predictable behavior. But I accept your arguments too, I would like to reflect a little more time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@betaboon I'm with @antongolub on this. My final position is that I'd rather have an override behavior with the CLI instead of extending it. Less bloat and noise – no need to merge all packages from different filtering places.
5ba9bb6
to
10d75b7
Compare
Is it possible to ignore changes to specifically |
I'm afraid, this feature is not provided at this moment. Now dependencies of all types are merged into one object. // Combine list of all dependency names.
const deps = Object.keys({
...manifest.dependencies,
...manifest.devDependencies,
...manifest.peerDependencies,
...manifest.optionalDependencies,
}); |
Hmm even if they were being combined like this, it wouldn't be too hard to conditionally add and/or exclude an entire category, or even specific packages. Would be a nice addition. |
allow defining
ignorePackages
inrelease.config.js
(and others) as discussed in #53