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

chore: reduce project.json duplication and split prepublish #2658

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kpanot
Copy link
Contributor

@kpanot kpanot commented Jan 6, 2025

Proposed change

chore: reduce project.json duplication
chore: split npm prepublish task

Related issues

- No issue associated -

Copy link

nx-cloud bot commented Jan 6, 2025

View your CI Pipeline Execution ↗ for commit bd00a83.

Command Status Duration Result
nx run-many --target=build --projects=eslint-pl... ✅ Succeeded <1s View ↗
nx run-many --target=build ✅ Succeeded 16m 42s View ↗
nx affected --target=lint ✅ Succeeded 15m 30s View ↗
nx affected --target=test --collectCoverage ✅ Succeeded 10m 5s View ↗
nx affected --target=package-github-action ✅ Succeeded 2m 26s View ↗
nx run-many --target=documentation ✅ Succeeded 1m 20s View ↗
nx run ama-sdk-schematics:build-swagger ✅ Succeeded <1s View ↗
nx run-many --target=build-swagger ✅ Succeeded 21s View ↗

☁️ Nx Cloud last updated this comment at 2025-01-10 01:12:15 UTC

@kpanot kpanot force-pushed the chore/nx-alignment branch from ee20a1a to 06e17db Compare January 6, 2025 15:00
},
"prepare-build-builders": {},
"build-builders": {},
"lint": {},
"test": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test": {
"test": {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, for a mysterious reason, it is mandatory in few packages (including this one).

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems that you had to add the executor for all the packages that have a test script in the package.json
this could be the mysterious reason, or just a coincidence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, not sure neither why having the script in the package.json would have any impact here

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why but I confirm that if you remove the test script in package.json you don't have to specify the executor
you can see when running yarn nx show project core --web that there is only one target test that appears either on npm scripts or others depending if you have a script with the name test or not in package.json

Copy link
Contributor Author

@kpanot kpanot Jan 10, 2025

Choose a reason for hiding this comment

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

Hmmm interesting, let's remove the test script then.
They are not really used

},
"prepare-build-builders": {},
"build-builders": {},
"lint": {},
"test": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test": {
"test": {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, for a mysterious reason, it is mandatory in few packages (including this one).

},
"prepare-build-builders": {},
"build-builders": {},
"lint": {},
"test": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test": {
"test": {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, for a mysterious reason, it is mandatory in few packages (including this one).

},
"prepare-build-builders": {},
"build-builders": {},
"lint": {},
"test": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test": {
"test": {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, for a mysterious reason, it is mandatory in few packages (including this one).

},
"prepare-build-builders": {},
"build-builders": {},
"lint": {},
"test": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test": {
"test": {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, for a mysterious reason, it is mandatory in few packages (including this one).

"lint": {
"executor": "nx:run-commands"
},
"lint": {},
"test": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test": {
"test": {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, for a mysterious reason, it is mandatory in few packages (including this one).

@kpanot kpanot force-pushed the chore/nx-alignment branch 2 times, most recently from 29ce3df to 3e07761 Compare January 7, 2025 06:15
@kpanot kpanot force-pushed the chore/nx-alignment branch 3 times, most recently from 7216b6a to 1f5f72d Compare January 7, 2025 08:32
nx.json Show resolved Hide resolved
@kpanot kpanot force-pushed the chore/nx-alignment branch from 1f5f72d to 6c50eb6 Compare January 7, 2025 14:23
@@ -16,15 +16,14 @@
"build:tools": "yarn nx run-many --target=build --projects=eslint-plugin,workspace",
"build:lint": "yarn nx run-many --target=build --projects=eslint-plugin",
"build:swagger-gen": "yarn nx run-many --target=build-swagger",
"prepare:publish": "yarn prepare-publish \"$(yarn workspaces:list)\" --append dist",
Copy link
Contributor

@fpaul-1A fpaul-1A Jan 7, 2025

Choose a reason for hiding this comment

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

The goal of this task was to fix #112, is this no longer applicable?

Copy link
Contributor Author

@kpanot kpanot Jan 7, 2025

Choose a reason for hiding this comment

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

It is still valid on Nx 19 (4s per prepare-publish task) but it is greatly reduced in Nx 20 (< 200ms).

@kpanot kpanot force-pushed the chore/nx-alignment branch 2 times, most recently from ebe2519 to 1871ddf Compare January 9, 2025 12:59
chore: split npm prepublish task
@kpanot kpanot force-pushed the chore/nx-alignment branch from 1871ddf to bd00a83 Compare January 10, 2025 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project:@ama-sdk/client-angular project:@ama-sdk/client-beacon project:@ama-sdk/client-fetch project:@ama-sdk/core project:@ama-sdk/create project:@ama-sdk/schematics project:@ama-sdk/swagger-builder project:@ama-terasu/cli project:@ama-terasu/core project:@ama-terasu/schematics project:audit-gh-action project:cascading-gh-action project:get-npm-tag-gh-action project:new-version-gh-action project:@o3r/amaterasu-api-spec project:@o3r/amaterasu-dodo project:@o3r/amaterasu-otter project:@o3r/amaterasu-sdk project:@o3r/analytics project:@o3r/apis-manager project:@o3r/application project:@o3r/artifactory-tools project:@o3r/azure-tools project:@o3r/build-helpers project:@o3r/chrome-devtools project:@o3r/components project:@o3r/configuration project:@o3r/core project:@o3r/create project:@o3r/design project:@o3r/dev-tools project:@o3r/dynamic-content project:@o3r/eslint-config project:@o3r/eslint-config-otter project:@o3r/eslint-plugin project:@o3r/extractors project:@o3r/forms project:@o3r/github-cascading-app project:@o3r/localization project:@o3r/logger project:@o3r/mobile project:@o3r/new-version project:@o3r/pipeline project:@o3r/routing project:@o3r/rules-engine project:@o3r/schematics project:@o3r/showcase project:@o3r/store-sync project:@o3r/storybook project:@o3r/stylelint-plugin project:@o3r/styling project:@o3r/telemetry project:@o3r/test-helpers project:@o3r/testing project:@o3r/third-party project:@o3r/workspace project:@o3r-training/showcase-sdk project:@o3r-training/training-tools project:otter-devtools project:release-gh-action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants