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(js): refactor ensurePackage #15074

Merged
merged 1 commit into from
Feb 22, 2023
Merged

Conversation

FrozenPandaz
Copy link
Collaborator

@FrozenPandaz FrozenPandaz commented Feb 16, 2023

Current Behavior

ensurePackage does not work with pnpm as the files that are currently executing may get moved entirely.

ensurePackage does more of its fair share:

  1. Installs the package into the workspace
  2. Allows requiring later on
  3. Doesn't update the Tree with the updated package.json yet appears after the generate command.

Expected Behavior

ensurePackage will work with pnpm.

  1. Installs the package into a tmp directory
  2. Allows requiring later on
  3. Will not accept a Tree anymore
  4. Will not update the Tree. That should be done via addDependenciesToPackageJson

Breaking Changes

ensurePackage will no longer install directly into the workspace. This must be done via the normal tools.

Related Issue(s)

Fixes #14933

@vercel
Copy link

vercel bot commented Feb 16, 2023

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

Name Status Preview Comments Updated
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 22, 2023 at 10:55PM (UTC)

@FrozenPandaz FrozenPandaz changed the title Ensure package2 fix(js): refactor ensurePackage Feb 16, 2023
packages/devkit/src/utils/package-json.ts Outdated Show resolved Hide resolved
packages/devkit/src/utils/package-json.ts Outdated Show resolved Hide resolved
packages/devkit/src/utils/package-json.ts Outdated Show resolved Hide resolved
packages/devkit/src/utils/package-json.ts Show resolved Hide resolved
packages/nx/src/utils/nx-plugin.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@nartc nartc left a comment

Choose a reason for hiding this comment

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

LGTM on nrwl/js side

Copy link
Contributor

@barbados-clemens barbados-clemens left a comment

Choose a reason for hiding this comment

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

Looks good from jest/cypress side

Copy link
Contributor

@meeroslav meeroslav left a comment

Choose a reason for hiding this comment

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

Linter part looks good!

Copy link
Contributor

@barbados-clemens barbados-clemens left a comment

Choose a reason for hiding this comment

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

cypress/jest look fine. just had a non-blocking question on if jest/cypress really need to call the jsInitGenerator.

Comment on lines +197 to +200
await jsInitGenerator(tree, {
...schema,
skipFormat: true,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just moving this around, but I was thinking about this and does @nrwl/jest really need to have the jsInitGenerator? Jest doesn't really care to have the root tsconfig just needs it's configurations? just a thought.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, we can investigate removing it.

I think jest needs it for tsconfig.spec.json?

@@ -1,3 +1,5 @@
export interface Schema {
skipPackageJson?: boolean;

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: extra space. nx format will probably catch it.

@FrozenPandaz FrozenPandaz merged commit fa6e8ea into nrwl:master Feb 22, 2023
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

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 3, 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.

Default generated js libraries cannot be dynamically installed & required.
8 participants