-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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(testing): move jest to nrwl devkit #4454
Conversation
4145035
to
201b4c2
Compare
Nx Cloud ReportCI ran the following commands for commit 3fa8432. Click to see the status, the terminal output, and the build insights.
Sent with 💌 from NxCloud. |
d64e062
to
76c1a1c
Compare
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.
Left a few comments, but overall looks good. Once addressed, you can merge the changes.
packages/devkit/src/utils/json.ts
Outdated
@@ -18,3 +18,16 @@ export function readJson<T = any>(host: Tree, path: string) { | |||
throw new Error(`Cannot parse ${path}: ${e.message}`); | |||
} | |||
} | |||
|
|||
export function writeJson<T = any>(host: Tree, path: string, val: T) { |
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.
Add API docs
host.write(path, JSON.stringify(val, null, 2)); | ||
} | ||
|
||
export function updateJson<T = any, U = T>( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
import { installPackagesTask } from '../tasks/install-packages-task'; | ||
|
||
/** | ||
* Add Dependencies and Dev Dependencies to package.json and queue an npm install if necessary |
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.
I'd not use the word queue cause we aren't queueing anything. Maybe say and returns a task that runs npm installs
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.
I'd also add a small example
/** | ||
* Add Dependencies and Dev Dependencies to package.json and queue an npm install if necessary | ||
*/ | ||
export function addDependenciesToPackageJson( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
/** | ||
* This reads a `migration.json` file and makes appropriate updates to dependency versions. | ||
*/ | ||
export function updatePackagesInPackageJson( |
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 hsoul dnot be required, right?
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 function was only needed for ng update, not for nx migrate
export type StringChange = StringInsertion | StringDeletion; | ||
|
||
/** | ||
* Use this to queue changes a string's original value and replay them |
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 function doesn't queue anything (especially given that you resort the changes) and it doesn't really "replay" stuff. I'd change it as applies a list of changes to a string and describe when it is needed. The position is being moved etc.
d: 'DELETE', | ||
}; | ||
|
||
class AdapterTree { |
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.
I'd rename it to show the direction of the wrapping. The reason is that we have so many things wrapping other things to make this integration seamless that I get lost. Something like NxDevkitTreeWrappingAngularDevkitTree.
const installTask = updateDependencies(tree, options); | ||
removeNrwlJestFromDeps(tree); | ||
updateExtensions(tree); | ||
if (installTask) { |
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.
You can also do return installTask
76c1a1c
to
da2625f
Compare
da2625f
to
3fa8432
Compare
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. |
Current Behavior
Jest uses the Angular Devkit
Expected Behavior
Jest uses the Nrwl Devkit
Related Issue(s)
Fixes #