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

@babylonjs/inspector and gui-editor packages install unnecessary dependencies / cause TS errors in certain project setups #13581

Open
wmurphyrd opened this issue Mar 2, 2023 · 5 comments

Comments

@wmurphyrd
Copy link
Contributor

Repro

  • Bug repro : npm install @babylonjs/inspector && npm ls @types/react
  • Expected result: └── (empty)
  • Current result:
└─┬ @babylonjs/[email protected]
  ├─┬ @babylonjs/[email protected]
  │ └── @types/[email protected] deduped
  ├─┬ @types/[email protected]
  │ └── @types/[email protected] deduped
  └── @types/[email protected]

npm 8.19.3

Additional context

Installing react types creates big problems in projects using typescript and non-react frameworks like vue. See e.g. vuejs/language-tools#592

These package are distributed as a pre-built bundles with the main and module fields pointing to dist files, so there's no need to include build-time dependencies, but npm installs these because they are listed as peer dependencies:

"peerDependencies": {
"@babylonjs/core": "^5.22.0",
"@babylonjs/gui": "^5.22.0",
"@babylonjs/gui-editor": "^5.22.0",
"@babylonjs/loaders": "^5.22.0",
"@babylonjs/materials": "^5.22.0",
"@babylonjs/serializers": "^5.22.0",
"@types/react": ">=16.7.3",
"@types/react-dom": ">=16.0.9"
},

Moving both @types/react and @types/react-dom from peerDependencies to devDependencies in these two packages would resolve the issue

@RaananW
Copy link
Member

RaananW commented Mar 2, 2023

The inspector typings depend on react, so those typings are needed in order to use the inspector as is. Building with the inspector will fail on (ts-enabled) projects that don't include those. npm has changed the way it treats peer dependencies (personally I find it better, but one could argue that it isn't ;-) ). So the dependency is installed.
I'll be happy to discuss it in the forum. i'll be happy to understand in what project this files and why.
i'm closing it for now (as we are using github issues exlusively to track work) and waiting for a ping on our forum.

@RaananW RaananW closed this as completed Mar 2, 2023
@wmurphyrd
Copy link
Contributor Author

wmurphyrd commented Mar 2, 2023

@RaananW types don't exist at runtime in javascript, so having type libraries as a runtime dependency is never useful

I'll leave this here for others that have the issue then since this is closed: the workaround described here for stubbing out the unnecessary react types library does work: vuejs/language-tools#592 (comment)

@RaananW
Copy link
Member

RaananW commented Mar 2, 2023

You will notice i did mention typescript in my answer. And the same goes to what you said - it shouldn't fail 🙃
But i do want to find a solution that works for everyone and will be really happy to discuss it longer in our forum, if possible

@RaananW RaananW reopened this Mar 2, 2023
@sebavan sebavan added the build label Mar 7, 2023
@bghgary bghgary added this to the 7.0 milestone Mar 7, 2023
@thomlucc thomlucc modified the milestones: 7.0, Future Jan 9, 2024
@thomlucc thomlucc modified the milestones: Future, 7.0 Mar 12, 2024
@RaananW
Copy link
Member

RaananW commented Mar 19, 2024

I did some research regarding what can be done here. I believe the es6 version could be exported without the react dependencies, if we change the public API available for the package consumer. However, I prefer not to make this change a week before our 7.0 release.
I am moving this to 8.0 for now.

@RaananW RaananW modified the milestones: 7.0, 8.0 Mar 19, 2024
Copy link

This issue has been automatically staled because it has been inactive for more than 14 days. Please update to "unstale".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants