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

Nodes: use relative references inside examples/jsm #27381

Merged

Conversation

hybridherbst
Copy link
Contributor

Fixes (at least partically)

Description

These are the only places where importmaps are currently required when accessing code from examples/jsm.
All other places that import from three/nodes are either inside example HTML files (that have importmaps) or inside the playground folder.

CC @sunag; is there a specific reason for why three/nodes is used? I understand that the location of this code may change again but I think then the paths should be adjusted instead of in the meantime requiring users to go through hoops with various bundlers to get these references to work.

@marcofugaro
Copy link
Contributor

marcofugaro commented Dec 14, 2023

I understand that the location of this code may change again but I think then the paths should be adjusted instead of in the meantime requiring users to go through hoops with various bundlers to get these references to work.

Could you share a test of this import being an issue?

is there a specific reason for why three/nodes is used?

We recommend the users to import from three/nodes rather than using the three/examples/jsm/nodes/Nodes.js path, both in the web environment (with import maps) and node environment (works by default). So having this import in the source code doesn't currently pose a problem.

@hybridherbst
Copy link
Contributor Author

I commented on the referenced issue. Once that is resolved happy to close this PR if really wanted.

@sunag
Copy link
Collaborator

sunag commented Dec 20, 2023

I think this PR follow part of work: #25904

@sunag sunag merged commit 3e1c5f4 into mrdoob:dev Dec 20, 2023
11 checks passed
AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
@LeviPesin
Copy link
Contributor

Can we please reconsider this?

Using relative paths feels really like a hack -- first, three/nodes is much more readable than a relative path, second, importing from a relative path may lead a user to an idea of importing a node directly from its file, which is NOT supported and may lead to breaking everything (only importing the main Nodes.js file is supported).
Third, hardcording paths significantly complicates just copying out a directory or two (core, nodes, renderers) from the repo and then easily allowing them to resolve each other using import maps -- I understand this might not be such an issue nowadays because of less usage of such a workflow, but still... Why hardcode paths when you may just not hardcode them?

@mrdoob @Mugen87 @sunag @hybridherbst @marcofugaro

@hybridherbst
Copy link
Contributor Author

@LeviPesin see #27354 for a lot of discussion on the "why" and the current status of bundlers respecting or ignoring the virtual paths defined in package.json.

I think calling relative paths "hardcoded" and virtual paths, that may or may not be resolved correctly depending on the environment, "not hardcoded" is a misnomer...

For all of our projects we're using explicit import paths for that very reason, the virtual paths three/addons and three/nodes are not usable in too many circumstances. I still think keeping three.js clean of virtualized paths is the better approach.

@LeviPesin
Copy link
Contributor

LeviPesin commented Jan 18, 2024

I think calling relative paths "hardcoded" and virtual paths, that may or may not be resolved correctly depending on the environment, "not hardcoded" is a misnomer...

I understand that some (quite old already) browsers don't support import maps -- that what is the polyfill for them for. But if bundlers have issues with supporting them -- I would consider this more of a bundlers problem rather than a problem of using virtual paths...

@hybridherbst
Copy link
Contributor Author

I would consider this more of a bundlers problem

I agree but it doesn't change that three.js is not usable in these cases. It's not the "dead 0.2%" of cases unfortunately, there's quite some popular combinations that currently don't support these virtual paths (e.g. vite + svelte/sveltekit). Where I can I already open issues with these bundlers.

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

Successfully merging this pull request may close these issues.

4 participants