-
Notifications
You must be signed in to change notification settings - Fork 925
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
fixed env config renaming #7823
base: master
Are you sure you want to change the base?
Conversation
config[targetId.toString()] = config[aspectId]; | ||
delete config[aspectId]; | ||
this.markAsChanged(); | ||
} | ||
if (aspectId === EnvsAspect.id) { | ||
const envConfig = config[aspectId]; | ||
if (envConfig !== REMOVE_EXTENSION_SPECIAL_SIGN && envConfig.env === sourceId.toString()) { | ||
if (envConfig !== REMOVE_EXTENSION_SPECIAL_SIGN && envConfig.env !== sourceId.toString()) { |
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.
Why this need be!== and not ===
With!== it will replace any env for any component to the new one
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.
because it works? 🤣
envConfig.env is the env prop in the config for the currently iterated component in the bitmap.
the sourceId (should change it to the fullSoruceId) is the env we want to change.
So you are right and I have to fix it now 😨 (I was testing it on one component)
Fixed on next commit. Also fixed a case where the source env is specified with a version and it wouldn't detect it.
const fullSourceId = this.getBitmapEntry(sourceId).id.toStringWithoutVersion(); | ||
let fullSourceId: string; | ||
try { | ||
fullSourceId = this.getBitmapEntry(sourceId).id.toStringWithoutVersion(); |
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.
Instead of try catch you can use
getBitmapEntryIfExist
And instead of doing toStringWithoutVersion
you can pass a second arg to the get like this:
{
"ignoreVersion": true
}
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.
Thanks
Actually, I realized that the sourceId is already a componentId so this whole get entry is just redundant.
@@ -192,19 +192,24 @@ export class BitMap { | |||
const config = componentMap.config; | |||
if (!config) return; | |||
Object.keys(config).forEach((aspectId) => { | |||
if (aspectId === sourceId.toString()) { | |||
const fullSourceId = sourceId.toStringWithoutVersion(); |
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.
how sourceId.toStringWithoutVersion();
becomes full source id?
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.
because it is passed as componentId as you can see in the method params:
async rename(sourceIdStr: string, targetName: string, options: RenameOptions): Promise<RenameDependencyNameResult> {
if (!isValidIdChunk(targetName)) {
if (targetName.includes('.'))
throw new Error(`error: new-name argument "${targetName}" includes a dot.
make sure this argument is the name only, without the scope-name. to change the scope-name, use --scope flag`);
throw new InvalidName(targetName);
}
const sourceId = await this.workspace.resolveComponentId(sourceIdStr); <-----
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.
ok, I understand, in that case the name of your var is confusing, it's not a full source id, but soruceIdWithoutVersion
@@ -192,19 +192,24 @@ export class BitMap { | |||
const config = componentMap.config; | |||
if (!config) return; | |||
Object.keys(config).forEach((aspectId) => { | |||
if (aspectId === sourceId.toString()) { | |||
const fullSourceId = sourceId.toStringWithoutVersion(); |
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.
ok, I understand, in that case the name of your var is confusing, it's not a full source id, but soruceIdWithoutVersion
if (aspectId === EnvsAspect.id) { | ||
const envConfig = config[aspectId]; | ||
if (envConfig !== REMOVE_EXTENSION_SPECIAL_SIGN && envConfig.env === sourceId.toString()) { | ||
if (envConfig !== REMOVE_EXTENSION_SPECIAL_SIGN && envConfig.env === fullSourceId) { |
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.
The envConfig.env
might have a value with a version, in that case, your new check will miss it.
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.
This check is for this prop:
"env": "teambit.react/react-env"
}
afaik it can't be with the version over there.
I think the issue you are referring to is with the env itself as a prop for example:
"teambit.react/[email protected]": {},
In that case, this one is resolved here:
if (aspectIdWithoutVersion === sourceIdWithoutVersion) {
Fix
teambit.envs/envs
Property: Ensures theteambit.envs/envs
prop inbitmap
is updated when renaming an environment. This correction handles the property in components using that environment.teambit.react/react-env
to justreact-env
during a rename operation.Changes
New Method in Workspace Aspect:
replaceEnvForAllComponents
method within the Workspace aspect. This method allows for the substitution of an environment for all components reliant on the old environment, returning the IDs of affected components.Updates to Rename Method:
rename
method ensure that it checks if a component is an environment component and then uses the newreplaceEnvForAllComponents
method.Bit Env Replace Command Update:
Refactoring:
replace env
command was extracted and moved to a new method on the workspace aspect. Both thereplace
andrename
methods utilize this.Removed Method:
renameAspectInConfig
method, previously used for renaming aspects in component configs, has been removed.Kindly review the changes and share your feedback.