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

feat: component marked as peer #8561

Merged
merged 38 commits into from
Feb 28, 2024

Conversation

zkochan
Copy link
Member

@zkochan zkochan commented Feb 22, 2024

Proposed Changes

@zkochan zkochan requested a review from GiladShoham February 23, 2024 16:48

async report([componentId, range]: [string, string]) {
await this.deps.setPeer(componentId, range != null ? range.toString() : range);
return '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write some success message here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

helper.command.build();
workspaceCapsulesRootDir = helper.command.capsuleListParsed().workspaceCapsulesRootDir;
});
it('should save the peer dependency in the model', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add another test with snap/tag and validate the component data after it
for both the always peer component (make sure that data of always peer is stored) and the dependent component (make sure it was stored with peer dep)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

const existingDependency = this.getExistingDependency(this.allDependencies.dependencies, dependency.id);
const existingDevDependency = this.getExistingDependency(this.allDependencies.devDependencies, dependency.id);
// no need to enter dev dependency to devDependencies if it exists already in dependencies
if (existingDependency || (existingDevDependency && fileType.isTestFile)) {
if (existingDependency || (existingDevDependency && opts.fileType.isTestFile)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what happens if you have a dependency without the always peer.
then you tag both components.
then you change the always peer to true.
maybe it will get into this condition and won't move to be peer?
worth validating

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test

Comment on lines +609 to +610
} else if (opts.isPeer) {
this.allDependencies.peerDependencies.push(currentComponentsDeps);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's used from a test file, should we make it dev dep or peer dep here?
that's a question. not sure about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be a dev dep in that case. Test files are not used when the package is installed as a dependency, which is when peer deps are useful.

@zkochan zkochan requested a review from GiladShoham February 28, 2024 09:45
@GiladShoham GiladShoham merged commit a9d86f3 into teambit:master Feb 28, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants