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

modularize.js class support #19061

Closed
wants to merge 3 commits into from
Closed

modularize.js class support #19061

wants to merge 3 commits into from

Conversation

DefinitelyMaybe
Copy link
Contributor

@DefinitelyMaybe DefinitelyMaybe commented Apr 6, 2020

relates to #11552

Initial regex proposal.
I wasn't sure why ([a-zA-Z0-9]+) was being used instead of (\w+). mdn link about character classes Happy to change that around later.

@DefinitelyMaybe
Copy link
Contributor Author

oops. realized that in examples/js/* land, the appropriate syntax would be:
THREE.classname = class classname

@mrdoob
Copy link
Owner

mrdoob commented Apr 7, 2020

I'm not sure I understand what this PR is intended to do...

@donmccurdy
Copy link
Collaborator

See #11552 (comment). I haven't tested to see if this works as intended yet, but the goal looks good to me:

Because modularize.js doesn't (currently) support ES6 Classes, we can't convert examples/js to use classes. Converting examples/js must happen before the files examples extend, in src/, are converted to classes. And converting src/ to classes must happen for tree-shaking, for reasons I still don't understand. 😅

@DefinitelyMaybe
Copy link
Contributor Author

DefinitelyMaybe commented Apr 7, 2020

So after this PR, I intend on working my way through examples/js/ and converting each script to use THREE.classname = class classname, at which point examples/jsm/ versions will still be generated as per beforehand.

@mrdoob
Copy link
Owner

mrdoob commented Apr 7, 2020

Because modularize.js doesn't (currently) support ES6 Classes, we can't convert examples/js to use classes.

Haven't read #11552 yet, but converting examples/js to classes sounds strange 🤔

@donmccurdy
Copy link
Collaborator

We can't convert src/ files to classes now because they're extended by files in examples/js and examples/jsm: ES classes can't be extended except by other ES classes.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 7, 2020

I think we would should only move forward with the class migration until it's clear what's happening with examples/js. If it is removed, we can also remove modularize.js and don't need this PR.

@donmccurdy
Copy link
Collaborator

It's no harder to convert examples/js/loaders/GLTFLoader to an ES class than to convert examples/jsm/loaders/GLTFLoader.

Deleting examples/js, if that is still the plan (wasn't it meant to happen in January?) could take a long time. I'm not aware that there are any clear next steps on that project, or who is doing them if so. I'd prefer not to get the (already complicated) tree-shaking problem blocked behind that.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 7, 2020

I'd prefer not to get the (already complicated) tree-shaking problem blocked behind that.

Yeah, that make sense. Modules and classes are two different things.

However, if the project decides to deprecate examples/js and remove it a few releases later, one can freeze the support for them. Meaning only the jsm files are maintained (and migrated to classes) and the deprecated js files are not changed anymore.

@donmccurdy
Copy link
Collaborator

I think that as long as both examples/js and examples/jsm are in the codebase, they should be functionally equivalent. Our steps should be:

(1) Add a deprecation warning to every file in examples/js.
(2) Wait a bit.
(3) Delete every file in examples/js.
(4) Begin updating files in examples/jsm directly.

I don't think there should be any middle stage where examples/js is in the codebase but missing updates that are in examples/jsm. That sounds likely to confuse people, and it's hard to predict how long that "deprecated but not yet deleted" stage will last if significant obstacles to the deprecation come up.

@DefinitelyMaybe
Copy link
Contributor Author

if it's ok with you guys, I might jump ahead and work through a couple of files as though this PR was merged. worst case, I'll have a bunch more PR's that wont make sense until this one is merged (which I can close if examples/js/ is suddenly depreciated)

@DefinitelyMaybe
Copy link
Contributor Author

Order of regex's matters. THREE.classname = class classname is more specific and voided if the // class name regex occurs first.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 8, 2020

if it's ok with you guys, I might jump ahead and work through a couple of files as though this PR was merged.

I think it's better to wait until the roadmap is clear.

@DefinitelyMaybe
Copy link
Contributor Author

sure. I'll create an issue for it to be discussed.

@munrocket
Copy link
Contributor

Tests is ok here.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 9, 2020

Closing, see #19073 (comment)

@Mugen87 Mugen87 closed this Apr 9, 2020
@DefinitelyMaybe
Copy link
Contributor Author

goodness. @Mugen87 could you please reopen this PR if after some thought and discussion we realize going down this route was actually an all good idea.

@mrdoob
Copy link
Owner

mrdoob commented Apr 10, 2020

@Mugen87 could you please reopen this PR if after some thought and discussion we realize going down this route was actually an all good idea.

We have reopened PRs in the past. Having them closed sends the signal to contributors that we currently don't think it's the way to go.

@DefinitelyMaybe
Copy link
Contributor Author

sure. hey, is it possible that I missed these nuances in a doc somewhere?

@DefinitelyMaybe DefinitelyMaybe deleted the modularize--class-support branch June 6, 2020 01:17
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.

5 participants