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(core): extract js related code from affected and hasher #16244

Merged
merged 9 commits into from
Apr 18, 2023

Conversation

meeroslav
Copy link
Contributor

@meeroslav meeroslav commented Apr 12, 2023

  • Extract JS related code from affected graph
  • Extract JS related code from hasher
  • Add deep imports to barrel export

Current Behavior

Expected Behavior

Related Issue(s)

Fixes #

@meeroslav meeroslav self-assigned this Apr 12, 2023
@vercel
Copy link

vercel bot commented Apr 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 18, 2023 7:55am

Copy link
Member

@mandarini mandarini left a 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!

packages/nx/src/plugins/js/index.ts Outdated Show resolved Hide resolved
packages/nx/src/plugins/js/index.ts Outdated Show resolved Hide resolved
packages/nx/src/plugins/js/index.ts Outdated Show resolved Hide resolved
packages/devkit/src/generators/to-js.ts Outdated Show resolved Hide resolved
packages/js/src/index.ts Outdated Show resolved Hide resolved
packages/js/src/index.ts Show resolved Hide resolved
packages/js/src/index.ts Outdated Show resolved Hide resolved
packages/js/src/utils/package-json/update-package-json.ts Outdated Show resolved Hide resolved
packages/js/src/utils/typescript/ast-utils.ts Outdated Show resolved Hide resolved
packages/linter/src/utils/versions.ts Outdated Show resolved Hide resolved
@@ -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) {
Copy link
Collaborator

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was not addressed.

Copy link
Contributor Author

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(
Copy link
Collaborator

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

Copy link
Contributor Author

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.

packages/nx-plugin/src/utils/versions.ts Outdated Show resolved Hide resolved
@@ -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');
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Contributor Author

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,
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Comment on lines 21 to 22
swcCoreVersion,
swcNodeVersion,
Copy link
Collaborator

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';

Copy link
Contributor Author

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) {
Copy link
Collaborator

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Contributor Author

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.

@meeroslav meeroslav requested a review from FrozenPandaz April 17, 2023 21:06
@FrozenPandaz FrozenPandaz merged commit 2dd59c3 into nrwl:master Apr 18, 2023
peppoasap pushed a commit to peppoasap/nx that referenced this pull request Apr 18, 2023
@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 Apr 24, 2023
@meeroslav meeroslav deleted the extract-affected-js branch April 27, 2023 09:53
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.

7 participants