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.

Sign up for GitHub

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

Jump to bottom

fix(core): enforce fixed versions only for apps with createPackageJson #16398

Merged
merged 1 commit into from
Apr 20, 2023
Merged

fix(core): enforce fixed versions only for apps with createPackageJson #16398

merged 1 commit into from
Apr 20, 2023

Conversation

meeroslav
Copy link
Contributor

Current Behavior

Expected Behavior

Related Issue(s)

Fixes #16293

@meeroslav meeroslav requested a review from vsavkin as a code owner April 19, 2023 09:54
@meeroslav meeroslav self-assigned this Apr 19, 2023
@vercel
Copy link

vercel bot commented Apr 19, 2023 •
edited
Loading

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Apr 19, 2023 3:08pm

@nx-cloud
Copy link

nx-cloud bot commented Apr 19, 2023 •
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f5f269b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 8 targets

Sent with 💌 from NxCloud.

jaysoo
jaysoo reviewed Apr 19, 2023
View reviewed changes
packages/nx/src/plugins/js/package-json/create-package-json.ts
version,
section: 'devDependencies' | 'dependencies'
) => {
return (
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a nitpick, but IMO it's more clear like this:

return isLibrary
  ? packageJson[section][packageName] ||
      rootPackageJson[section]?.[packageName] ||
      version
  : packageJson[section][packageName] || version;

Or this?

if (isLibrary) 
  return packageJson[section][packageName] ||
      rootPackageJson[section]?.[packageName] ||
      version;
else
  return packageJson[section][packageName] || version;

Reasoning is that having isLibrary in the middle is harder to pick up at a quick glance. Putting it at the top separate the check for lib vs app, and finding default versions.

jaysoo
jaysoo reviewed Apr 19, 2023
View reviewed changes
packages/nx/src/plugins/js/package-json/create-package-json.ts
packageJson.dependencies[packageName] = version;
return;
}
if (!isLibrary) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine for now, but if the app vs lib logic deviates more, we might consider separating them into two functions: createAppPackageJson and createLibPackgeJson so the consumer can choose which to call depending on use-case.

jaysoo
jaysoo reviewed Apr 19, 2023
View reviewed changes
packages/nx/src/plugins/js/package-json/create-package-json.spec.ts
});

it('should override range versions with local ranges when creating package json for libs', () => {
spies.push(
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of having to mock these functions, but since it's used in existing tests let's go with it for now. Maybe something we can discuss later is testing strategies

@meeroslav meeroslav merged commit 4a4fc19 into nrwl:master Apr 20, 2023
meeroslav added a commit to meeroslav/nx that referenced this pull request Apr 20, 2023
@meeroslav
chore(repo): merge master to branch
5f5370c 
Squashed commit of the following:

commit a51ac58
Author: Miroslav Jonaš <[email protected]>
Date:   Thu Apr 20 09:16:45 2023 +0200

    fix(core): do not expect each package to have hoisted version in lockfile (nrwl#16393)

commit 4a4fc19
Author: Miroslav Jonaš <[email protected]>
Date:   Thu Apr 20 09:16:32 2023 +0200

    fix(core): enforce fixed versions only for apps with createPackageJson (nrwl#16398)

commit 8400484
Author: Emily Xiong <[email protected]>
Date:   Wed Apr 19 20:36:11 2023 -0400

    feat(core): update create-nx-plugin to generate cli library (nrwl#15994)

commit 16e4061
Author: Craigory Coppola <[email protected]>
Date:   Wed Apr 19 18:25:41 2023 -0400

    feat(devkit): bump support range to be 15 <= x <= 17 (nrwl#16421)

commit fc07794
Author: Craigory Coppola <[email protected]>
Date:   Wed Apr 19 18:07:56 2023 -0400

    feat(core): remove deprecated affected commands (nrwl#16349)

commit a798576
Author: Craigory Coppola <[email protected]>
Date:   Wed Apr 19 17:02:34 2023 -0400

    fix(misc): pass e2eTestRunner to child preset generator (nrwl#16414)

commit 338dc64
Author: Nicholas Cunningham <[email protected]>
Date:   Wed Apr 19 13:29:31 2023 -0600

    feat(nextjs): Add standalone Nextjs option to react selection when running CNW (nrwl#16317)

commit 16e115f
Author: Emily Xiong <[email protected]>
Date:   Wed Apr 19 15:03:36 2023 -0400

    fix(react-native): log errors for pod install (nrwl#16386)

commit 0fc3d44
Author: FrozenPandaz <[email protected]>
Date:   Wed Apr 19 14:24:10 2023 -0400

    chore(misc): publish 16.0.0-beta.4

commit f7ca6e6
Author: Craigory Coppola <[email protected]>
Date:   Wed Apr 19 14:21:30 2023 -0400

    chore(misc): update migration version for workspace-generator (nrwl#16417)

commit b3ca503
Author: Craigory Coppola <[email protected]>
Date:   Wed Apr 19 14:12:09 2023 -0400

    fix(core): hide duplicate log entry when displaying generator help (nrwl#16353)

commit 9fa6d41
Author: Craigory Coppola <[email protected]>
Date:   Wed Apr 19 14:06:57 2023 -0400

    fix(misc): mark workspace-lint as deprecated in yargs config (nrwl#16385)

commit 9c9abaa
Author: Emily Xiong <[email protected]>
Date:   Wed Apr 19 14:03:45 2023 -0400

    fix(react-native): remove listDevices default value for run-android (nrwl#16410)

commit 1743ff1
Author: Craigory Coppola <[email protected]>
Date:   Wed Apr 19 14:03:00 2023 -0400

    feat(core): add migration to update workspace generators to a local plugin (nrwl#12700)

commit f04f316
Author: Jack Hsu <[email protected]>
Date:   Wed Apr 19 14:00:24 2023 -0400

    feat(react): add rspack experimental support (nrwl#16252)

commit 1ff450b
Author: Jason Jean <[email protected]>
Date:   Wed Apr 19 13:43:44 2023 -0400

    fix(core): fix devkit compatibility (nrwl#16413)

commit 6677a9c
Author: Leosvel Pérez Espinosa <[email protected]>
Date:   Wed Apr 19 18:32:51 2023 +0100

    feat(misc): expose nx init command flags (nrwl#16287)

commit 5d7ad34
Author: Leosvel Pérez Espinosa <[email protected]>
Date:   Wed Apr 19 18:22:52 2023 +0100

    feat(testing): bump ts-jest to 29.1.0 to support typescript 5.x (nrwl#16401)

commit a8e85da
Author: Dominik Pieper <[email protected]>
Date:   Wed Apr 19 19:01:50 2023 +0200

    feat(testing): export cypressE2EConfigurationGenerator (nrwl#16394)

commit 1ad7e3d
Author: Caleb Ukle <[email protected]>
Date:   Wed Apr 19 11:55:11 2023 -0500

    fix(testing): nullcheck polyfills in ng component testing (nrwl#16411)

commit c963393
Author: Katerina Skroumpelou <[email protected]>
Date:   Wed Apr 19 19:34:38 2023 +0300

    fix(storybook): dont use swc addon on sb7 and format (nrwl#16408)

commit c8960b7
Author: Miroslav Jonaš <[email protected]>
Date:   Wed Apr 19 17:13:10 2023 +0200

    fix(linter): don't confuse buildable libs for secondary ng entry point (nrwl#16367)

commit 2734401
Author: FrozenPandaz <[email protected]>
Date:   Wed Apr 19 11:10:19 2023 -0400

    chore(misc): publish 16.0.0-beta.3

commit da158bc
Author: Jason Jean <[email protected]>
Date:   Wed Apr 19 10:59:58 2023 -0400

    feat(angular): replace usages of @nrwl with @nx (nrwl#16377)

    Co-authored-by: Colum Ferry <[email protected]>

commit 76dfc62
Author: Jason Jean <[email protected]>
Date:   Wed Apr 19 10:59:41 2023 -0400

    feat(web): replace usages of @nrwl with @nx (nrwl#16376)

commit 1780aac
Author: Jack Hsu <[email protected]>
Date:   Wed Apr 19 10:57:34 2023 -0400

    feat(react): add @nx/web as a dependency since it is needed by cypress/etc. (nrwl#16407)

commit 52acf07
Author: Jason Jean <[email protected]>
Date:   Wed Apr 19 10:40:37 2023 -0400

    fix(angular): fix resolve builder in ngcli adapter (nrwl#16375)

commit ab609a2
Author: Jack Hsu <[email protected]>
Date:   Wed Apr 19 10:38:01 2023 -0400

    fix(js): update swc options so path mappings can work in all environments (nrwl#16390)

commit 157b35b
Author: Katerina Skroumpelou <[email protected]>
Date:   Wed Apr 19 17:33:10 2023 +0300

    fix(storybook): do not override existing v7 on init (nrwl#16405)
@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 26, 2023
@meeroslav meeroslav deleted the fix-16293 branch April 27, 2023 09:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AgentEnder AgentEnder AgentEnder approved these changes

@jaysoo jaysoo jaysoo approved these changes

@vsavkin vsavkin Awaiting requested review from vsavkin

@FrozenPandaz FrozenPandaz Awaiting requested review from FrozenPandaz

@nartc nartc Awaiting requested review from nartc

@ndcunningham ndcunningham Awaiting requested review from ndcunningham

Assignees

@meeroslav meeroslav

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow for generated package.json to include dependency range from root package.json
3 participants
@meeroslav @jaysoo @AgentEnder