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 ts typing for CSS2DRenderer in example/jsm/renderer ( JS file not converted yet ) #16342

Closed
wants to merge 8 commits into from

Conversation

linonetwo
Copy link
Contributor

@linonetwo linonetwo commented Apr 27, 2019

I just want to help /jsm to complete soon

This fixes #16337

Tested locally.

@gkjohnson
Copy link
Collaborator

Awesome!

Hand converting examples to jsm isn't currently sustainable because it requires maintaining both the original and new files as updates are made. For the moment I think it would be preferred to modify the modularize.js script to automatically convert the CSS2DRenderer files, instead.

PR #15599 would enable a path forward for this type of manual conversion but it's unclear whether that will be merged.

/cc @donmccurdy

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 28, 2019

Um, when using modularize.js, it seems the script is not able to export CSS2DObject and CSS2DRenderer in a single file. Looks like it needs an enhancement...

@linonetwo
Copy link
Contributor Author

linonetwo commented Apr 28, 2019

Oh, I'm new to threejs, didn't know there is an auto converter, I will take a look at it.
I'll remove the .js file, and only keep .d.ts, seems your converter can't generate typing.

And I'm going to put two class into a single .d.ts file, since you are going to export CSS2DObject and CSS2DRenderer in a single file.

@linonetwo linonetwo changed the title Add CSS 2d renderer to example/jsm Add ts typing for CSS2DRenderer in example/jsm/renderer ( JS file not converted yet ) Apr 28, 2019
@gkjohnson
Copy link
Collaborator

@Mugen87 I may sound a bit like a broken record but I would be concerned that updating and maintaining that script will be error prone if we try to support every case needed to convert every file.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 29, 2019

Yeah, makes sense. I just thought if it's possible to convert a given file with modularize.js by implementing an appropriate enhancement, that should be the preferred way.

@donmccurdy
Copy link
Collaborator

I agree we're going to need #15599 or something like it. I tried converting this file a while ago and hit the same problems.

We started with everything in the global namespace, and we want to end up in a state where everything is an ES module and the legacy version is generated from that. Trying to create a middle step where every ES module can be generated from the global version automatically is a very tough – and avoidable – obstacle to the end goal.

/cc @mrdoob see #15599 (comment) 😇

@Mugen87
Copy link
Collaborator

Mugen87 commented May 17, 2019

#16472 added CSS2DRenderer as a module and the respective TS file.

BTW: The code base does not use the keywords public and private so far. Besides, module internal functions like getDistanceToSquared() or zOrder() are usually not respected by the declaration file. Just the public API.

Anyway, if you find a bug in the new TS files, it would be great if you make PRs with respective fixes^^.

@linonetwo
Copy link
Contributor Author

Glad to see those modules added, closing this PR.

@linonetwo linonetwo closed this May 17, 2019
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.

Export CSS2DRenderer in jsm
4 participants