Navigation Menu

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

fix(testing): fix generating cypress-project with no project specified #2965

Merged
merged 1 commit into from Jun 5, 2020

Conversation

devinshoemaker
Copy link
Contributor

@devinshoemaker devinshoemaker commented May 8, 2020

Current Behavior (This is the behavior we have today, before the PR is merged)

If a Cypress project is generated without a project specified then it will cause issues with the modified nx.json because the projects implicitDependencies will contain a null.

Expected Behavior (This is the new behavior we can expect after the PR is merged)

I am able to generate a Cypress project without a project specified.

Issue

Copy link
Collaborator

@FrozenPandaz FrozenPandaz left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Requested some minor changes.

@@ -49,7 +49,7 @@ function updateNxJson(options: CypressProjectSchema): Rule {
return updateJsonInTree<NxJson>('nx.json', (json) => {
json.projects[options.projectName] = {
tags: [],
implicitDependencies: [options.project],
implicitDependencies: options.project ? [options.project] : [],
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be consistent with other projects without implicitDependencies, I think it would be better to not have the option at all.

Currently, you can use a noop() rule in the default function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you not want to add the empty tags either in this case? So just don't execute the updateNxJson(...) method at all if no project is specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After re-reading your message I am pretty positive that's what you meant so I went ahead and made the changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I was mistaken... you are right we do need the tags. The expected result should be:

"my-app-e2e": {
  tags: []
},

@@ -111,7 +111,7 @@ export default function (options: CypressProjectSchema): Rule {
}),
generateFiles(options),
updateWorkspaceJson(options),
updateNxJson(options),
options.project ? updateNxJson(options) : noop(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, we do need tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think it's working like you suggested. Let me know if it looks good or if you want some slightly different syntax or something.

@devinshoemaker
Copy link
Contributor Author

@FrozenPandaz do these changes match what you were expecting? Let me know if you need me to tweak anything else prior to merging.

@FrozenPandaz
Copy link
Collaborator

FrozenPandaz commented May 15, 2020

Sorry, you were right initially, we do need the tags in there but not implicitDependencies.

Sorry, I was mistaken... you are right we do need the tags. The expected result should be:

"my-app-e2e": {
  tags: []
},

I was wrong about using the noop().

@devinshoemaker
Copy link
Contributor Author

@FrozenPandaz changes have been made, tests has been updated, and branch has been rebased.

@FrozenPandaz
Copy link
Collaborator

Thank you!

@FrozenPandaz FrozenPandaz merged commit 1f90080 into nrwl:master Jun 5, 2020
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants