-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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(core): extract js related code from affected and hasher #16244
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
4fa6a96
to
5ad4b1f
Compare
deea028
to
ee88d28
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.
Storybook, Vite and React changes look good to me!
3677795
to
2900b39
Compare
@@ -1,5 +1,5 @@ | |||
import { addDependenciesToPackageJson, formatFiles, Tree } from '@nrwl/devkit'; | |||
import { swcCoreVersion, swcNodeVersion } from 'nx/src/utils/versions'; | |||
import { swcCoreVersion, swcNodeVersion } from '../../utils/versions'; | |||
import { WORKSPACE_PLUGIN_DIR } from '../../generators/workspace-rules-project/workspace-rules-project'; | |||
|
|||
export default async function addSwcNodeIfNeeded(tree: Tree) { |
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.
Expose a function from @nrwl/js
which adds @swc/node
as a dependency.
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 was not addressed.
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 got lost on rebase; I'll push those changes again.
@@ -4,7 +4,7 @@ import { | |||
installPackagesTask, | |||
Tree, | |||
} from '@nrwl/devkit'; | |||
import { swcCoreVersion, swcNodeVersion } from 'nx/src/utils/versions'; | |||
import { swcCoreVersion, swcNodeVersion } from '../../utils/versions'; | |||
|
|||
export default async function addSwcNodeIfNeeded(tree: Tree) { | |||
addDependenciesToPackageJson( |
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.
Same as above, use the util from @nrwl/js
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 got lost on rebase; I'll push those changes again.
2900b39
to
e880f11
Compare
e2f991b
to
6044b00
Compare
6044b00
to
3d85ac0
Compare
e2e/jest/src/jest.test.ts
Outdated
@@ -48,7 +48,7 @@ describe('Jest', () => { | |||
updateFile( | |||
`libs/${mylib}/setup.ts`, | |||
stripIndents` | |||
const { registerTsProject } = require('nx/src/utils/register'); | |||
const { registerTsProject } = require('nx/src/plugins/js/utils/register'); |
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.
followup: This should use @nrwl/js/src/internals
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.
That is currently impossible since there is no src/internals
in the @nrwl/js
. Deep imports must be done using @nx/*
packages.
Additionally, using @nx/...
would not work in pnpm
since none of the @nx/*
packages are hoisted.
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.
Something to discuss tomorrow...
typescriptESLintVersion, | ||
} from '../../utils/versions'; | ||
import { nxVersion } from '@nx/js/src/utils/versions'; |
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 did this change?
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 thought it made sense to have nx as a central source of truth regarding nxVersion.
I'll revert that.
import { swcCoreVersion, swcNodeVersion } from 'nx/src/utils/versions'; | ||
import { nxVersion } from '../../utils/versions'; | ||
import { | ||
nxVersion, |
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.
What's wrong with using the one from this package?
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 thought it made sense to have nx as a central source of truth regarding nxVersion.
I'll revert that.
swcCoreVersion, | ||
swcNodeVersion, |
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 don't think we should import the versions from @nx/js
, I think we should import a function like { addSwcDependencies } from '@nx/js/src/utils/add-swc';
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 got lost on rebase; I'll push those changes again.
@@ -1,5 +1,5 @@ | |||
import { addDependenciesToPackageJson, formatFiles, Tree } from '@nrwl/devkit'; | |||
import { swcCoreVersion, swcNodeVersion } from 'nx/src/utils/versions'; | |||
import { swcCoreVersion, swcNodeVersion } from '../../utils/versions'; | |||
import { WORKSPACE_PLUGIN_DIR } from '../../generators/workspace-rules-project/workspace-rules-project'; | |||
|
|||
export default async function addSwcNodeIfNeeded(tree: Tree) { |
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 was not addressed.
@@ -1,7 +1,4 @@ | |||
export const nxVersion = require('../../package.json').version; |
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 remove 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.
I thought it makes sense to have nx
as a central source of truth when it comes to nxVersion
.
I'll revert that.
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. |
affected
graphhasher
Current Behavior
Expected Behavior
Related Issue(s)
Fixes #