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

Add optimize CLI join flags --join-keep-named and --join-keep-meshes #1551

Merged
merged 6 commits into from
Nov 19, 2024

Conversation

jo-chemla
Copy link
Contributor

#1550

Set default values to false like the current join CLI.

@jo-chemla jo-chemla changed the title Add optimize CLI join flags --join-keepNamed and --join-keepMeshes Add optimize CLI join flags --join-keep-named and --join-keep-meshes Nov 7, 2024
Copy link
Owner

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Thanks @jo-chemla! I'm open to adding these flags — note that they'll need to be passed through in the command implementation below these flag declarations.

@donmccurdy donmccurdy added feature New enhancement or request package:cli labels Nov 8, 2024
@donmccurdy donmccurdy added this to the v4.1 milestone Nov 8, 2024
@jo-chemla
Copy link
Contributor Author

🤦
Of course, sorry about that, was a bit distracted. Just pushed a commit that do use these flags as options when pushing the join transform to the array.

@jo-chemla
Copy link
Contributor Author

Should be good to go, tested via npm install --legacy-peer-deps && npx packages\cli optimize --join-keep-named true input.glb output.gltf and I confirm the behavior is correct.

Note the defaults are not displayed anymore in the respective help CLIs.

@donmccurdy donmccurdy merged commit 46ad981 into donmccurdy:main Nov 19, 2024
5 checks passed
@donmccurdy
Copy link
Owner

Thanks @jo-chemla! Just as FYI, I think I may rename these flags as --join-meshes and --join-named before the release (with inverted values).

Note the defaults are not displayed anymore in the respective help CLIs.

Hm, maybe it just doesn't show defaults whose values are 'false'? Annoying but outside the control of my library at tho moment. 😕

tested via npm install --legacy-peer-deps ...

Just curious, is --legacy-peer-deps needed for some reason?

@jo-chemla
Copy link
Contributor Author

Thanks for the feedback!

  • noted for the inversion of --join-xyz upon release
  • when I said defaults are not shown,
  • and the full log of npm i without --legacy-peer-deps is below, yields conflicting eslint dependencies, full CLI output below

Regarding defaults not printed indeed only those that are true are printed - first I thought this was because the default from code is a constant rather than explicit bool, default: !JOIN_DEFAULTS.keepNamed, rather than default: true,

image

And for the npm i log

C:\Dev\Iconem\glTF-Transform>npm i
npm warn ERESOLVE overriding peer dependency
npm warn While resolving: @typescript-eslint/[email protected]
npm warn Found: [email protected]
npm warn node_modules/eslint
npm warn   dev eslint@"9.7.0" from the root project
npm warn   4 more (@eslint-community/eslint-utils, eslint-compat-utils, ...)
npm warn
npm warn Could not resolve dependency:
npm warn peer eslint@"^8.56.0" from @typescript-eslint/[email protected]
npm warn node_modules/@typescript-eslint/eslint-plugin
npm warn   @typescript-eslint/eslint-plugin@"7.18.0" from [email protected]
npm warn   node_modules/typescript-eslint
npm warn
npm warn Conflicting peer dependency: [email protected]
npm warn node_modules/eslint
npm warn   peer eslint@"^8.56.0" from @typescript-eslint/[email protected]
npm warn   node_modules/@typescript-eslint/eslint-plugin
npm warn     @typescript-eslint/eslint-plugin@"7.18.0" from [email protected]
npm warn     node_modules/typescript-eslint
npm warn ERESOLVE overriding peer dependency
npm warn While resolving: @typescript-eslint/[email protected]
npm warn Found: [email protected]
npm warn node_modules/eslint
npm warn   dev eslint@"9.7.0" from the root project
npm warn   4 more (@eslint-community/eslint-utils, eslint-compat-utils, ...)
npm warn
npm warn Could not resolve dependency:
npm warn peer eslint@"^8.56.0" from @typescript-eslint/[email protected]
npm warn node_modules/@typescript-eslint/parser
npm warn   peer @typescript-eslint/parser@"^7.0.0" from @typescript-eslint/[email protected]
npm warn   node_modules/@typescript-eslint/eslint-plugin
npm warn   1 more (typescript-eslint)
npm warn
npm warn Conflicting peer dependency: [email protected]
npm warn node_modules/eslint
npm warn   peer eslint@"^8.56.0" from @typescript-eslint/[email protected]
npm warn   node_modules/@typescript-eslint/parser
npm warn     peer @typescript-eslint/parser@"^7.0.0" from @typescript-eslint/[email protected]
npm warn     node_modules/@typescript-eslint/eslint-plugin
npm warn     1 more (typescript-eslint)
npm warn ERESOLVE overriding peer dependency
npm warn While resolving: @typescript-eslint/[email protected]
npm warn Found: [email protected]
npm warn node_modules/eslint
npm warn   dev eslint@"9.7.0" from the root project
npm warn   4 more (@eslint-community/eslint-utils, eslint-compat-utils, ...)
npm warn
npm warn Could not resolve dependency:
npm warn peer eslint@"^8.56.0" from @typescript-eslint/[email protected]
npm warn node_modules/@typescript-eslint/type-utils
npm warn   @typescript-eslint/type-utils@"7.18.0" from @typescript-eslint/[email protected]
npm warn   node_modules/@typescript-eslint/eslint-plugin
npm warn
npm warn Conflicting peer dependency: [email protected]
npm warn node_modules/eslint
npm warn   peer eslint@"^8.56.0" from @typescript-eslint/[email protected]
npm warn   node_modules/@typescript-eslint/type-utils
npm warn     @typescript-eslint/type-utils@"7.18.0" from @typescript-eslint/[email protected]
npm warn     node_modules/@typescript-eslint/eslint-plugin
npm warn ERESOLVE overriding peer dependency
npm warn While resolving: @typescript-eslint/[email protected]
npm warn Found: [email protected]
npm warn node_modules/eslint
npm warn   dev eslint@"9.7.0" from the root project
npm warn   4 more (@eslint-community/eslint-utils, eslint-compat-utils, ...)
npm warn
npm warn Could not resolve dependency:
npm warn peer eslint@"^8.56.0" from @typescript-eslint/[email protected]
npm warn node_modules/@typescript-eslint/utils
npm warn   @typescript-eslint/utils@"7.18.0" from @typescript-eslint/[email protected]
npm warn   node_modules/@typescript-eslint/eslint-plugin
npm warn   2 more (@typescript-eslint/type-utils, typescript-eslint)
npm warn
npm warn Conflicting peer dependency: [email protected]
npm warn node_modules/eslint
npm warn   peer eslint@"^8.56.0" from @typescript-eslint/[email protected]
npm warn   node_modules/@typescript-eslint/utils
npm warn     @typescript-eslint/utils@"7.18.0" from @typescript-eslint/[email protected]
npm warn     node_modules/@typescript-eslint/eslint-plugin
npm warn     2 more (@typescript-eslint/type-utils, typescript-eslint)
npm error code ERESOLVE
npm error ERESOLVE could not resolve
npm error
npm error While resolving: [email protected]
npm error Found: [email protected]
npm error node_modules/eslint
npm error   dev eslint@"9.7.0" from the root project
npm error   peer eslint@"^6.0.0 || ^7.0.0 || >=8.0.0" from @eslint-community/[email protected]
npm error   node_modules/@eslint-community/eslint-utils
npm error     @eslint-community/eslint-utils@"^4.4.0" from @typescript-eslint/[email protected]
npm error     node_modules/@typescript-eslint/utils
npm error       @typescript-eslint/utils@"7.18.0" from @typescript-eslint/[email protected]
npm error       node_modules/@typescript-eslint/eslint-plugin
npm error         @typescript-eslint/eslint-plugin@"7.18.0" from [email protected]
npm error         node_modules/typescript-eslint
npm error       2 more (@typescript-eslint/type-utils, typescript-eslint)
npm error     @eslint-community/eslint-utils@"^4.2.0" from [email protected]
npm error     1 more (eslint-plugin-svelte)
npm error   3 more (eslint-compat-utils, eslint-config-prettier, eslint-plugin-svelte)
npm error
npm error Could not resolve dependency:
npm error peer eslint@"^8.56.0" from [email protected]
npm error node_modules/typescript-eslint
npm error   dev typescript-eslint@"^7.17.0" from the root project
npm error
npm error Conflicting peer dependency: [email protected]
npm error node_modules/eslint
npm error   peer eslint@"^8.56.0" from [email protected]
npm error   node_modules/typescript-eslint
npm error     dev typescript-eslint@"^7.17.0" from the root project
npm error
npm error Fix the upstream dependency conflict, or retry
npm error this command with --force or --legacy-peer-deps
npm error to accept an incorrect (and potentially broken) dependency resolution.
npm error
npm error
npm error For a full report see:
npm error C:\Users\jonathan\AppData\Local\npm-cache\_logs\2024-11-20T07_53_58_297Z-eresolve-report.txt
npm error A complete log of this run can be found in: C:\Users\jonathan\AppData\Local\npm-cache\_logs\2024-11-20T07_53_58_297Z-debug-0.log

@donmccurdy
Copy link
Owner

... and the full log of npm i without --legacy-peer-deps is below, yields conflicting eslint dependencies

Ah ok, thanks! I use Yarn, perhaps this is needed since npm presumably isn't reading the yarn.lock file then.

@jo-chemla
Copy link
Contributor Author

Indeed, yarn did handle package resolution on its own with no additional steps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New enhancement or request package:cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants