Material: use class properties #24237
Conversation
|
Shouldn't we then also do the same with MeshPhysicalMaterial ( |
|
What are the benefits of doing this? Is it only syntactic sugar? I have the feeling it's still too early for this. We should not assume that all devs use the latest versions of their bundlers. And depending on the project, it's not always possible to upgrade to the latest bundler version. |
Better tree-shaking for other bundlers that are not rollup.
Ok, what timeline are you proposing? |
|
I wish I could make a precise suggestion here... The problem is that this kind of syntax potentially breaks builds. And there is normally no other solution than upgrading the build-tool (which might not be possible in all scenarios). I think I would only introduce this kind of syntax if there is a compelling reason. Just further improving tree-shaking capabilities seems not appropriate to me if we risk to break other projects. |
|
I agree that it might break some builds. But my point of view is that if people are willing to upgrade three in their existing projects, they are willing to upgrade their bundler as well. Webpack 5 has been out since Oct 2020. |
I remember Safari being a problem too 🫠 |
One benefit of this is that we don't pollute the class and when the dev console.logs and object the list of properties is cleaner. |
|
That's true! I'm just afraid about the breaking character of this change. |
|
Oh yeah, definitely... |
|
Ok, iOS Safari supports class fields since version
Safari for desktop support it from version
Keep in mind that in term of support there is no distinction between private, public or static. It's all supported the same. |
I think it's a good move for code robustness. e.g. Vector3 uses Euler's privates by accident Lines 664 to 672 in 90165b4 would be better to declare them as private fields in Euler: class Euler {
#x = 0
#y = 0
#z = 0so err can be detected by lsp, or on running tests e.g. WebglBindingStates sets a InstancedBufferGeometry's private which is not even declared three.js/src/renderers/webgl/WebGLBindingStates.js Lines 392 to 396 in 90165b4 would be better to declare it as internal public field in InstancedBufferGeometry, like class InstancedBufferGeometry {
/** @internal **/
maxInstanceCount = undefined
}or, serialization-friendlyer: class InstancedBufferGeometry {
#maxInstanceCount = undefined
setMax(v) {..}
getMax() {..}
} |
|
@Mugen87, would you be more comfortable with this change if we tested for compat with previously problematic tools? Are there any besides Webpack 4-5 (CRA) and Safari? This is stable in Node itself since v12, so this isn't imposed on the runtime, as far as tooling. This is a reoccurring question, and one might want a https://caniuse.com for the state of tooling. CDNs and services like https://skypack.dev and https://bundlephobia.com could also be of note. Importantly, I've found the latter to break on optional chaining (es2020 -- see esbuild's guide on JS features/version), so I had to transpile to es2018. If this is still the case, it would be good to report ahead of time for a smooth rollout. |
|
For Bundlephobia it has already been reported: pastelsky/bundlephobia#559 (they are using webpack 4). I don't think that tool is being maintained that much... |
|
I don't have any particular opinion about class properties, but I wish we could address these discussions with a policy on browser and bundler support, and (ideally) set up tests against that. For example:
|
|
We still transpile to es2018 around pmndrs for things like class properties, nullish coalescing, etc. (this is an issue for Webpack 4 and older Node versions), but there might be related tooling that accepts a browserslist with a policy you're describing. The real point of tension is that this isn't automated via transpilation for addons, so we have to resort to lint rules (#23763) and integration tests (not implemented AFAIK) whereas it can be automatic for the core. I'm not sure how helpful the linter has been, but I hoped it would enforce a policy like this. |
|
I'm glad to have the linter, it simplifies these conversations generally. I guess the issue here is that we're discussing a feature the listed browsers already support, but the bundlers maybe don't, so we're flying blind on that a bit. If targeting a particular EcmaScript version would work better i'm all for that, but wasn't sure if it'd line up cleanly enough with browser and/or bundler support. |
|
Is there someone who will volunteer to manually test this PR (and #25104) against Webpack, Parcel, Vite, ESBuild? That way we know, and we can move forward. |
|
I have a similar repo that was testing tree-shaking with a bunch of different tooling in troika https://github.com/CodyJasonBennett/treeshaking (inspired by efforts over here), but this PR still doesn't play nice with Webpack 4. I resolved similar with pmndrs/meshline today for Webpack 4 (CRA, Storybook) specifically. |
Transpilation was brought up in the past, the core maintainers are against it.
Ok! I tested and confirm that this PR (and #25104) works with all current versions of the bundlers. ✅
*Or even before, I wasn't able to test previous versions because old esbuild doesn't support arm64 mac. I'll let @mrdoob and @Mugen87 decide which version back they want to support, like @donmccurdy proposed. |
|
You're completely ignoring my comment and Don's. This comes across as misleading and needlessly contrarian, and I've seen a history of this from you. I apologize if there's a language barrier but this is burning me out of further discussion. |
But is Webpack 4 currently used much (except in Bundlephobia)? |
Yes, and this is a constant conversation with big frameworks being likely culprits. I'm happy to go over trends to gauge progress or timeline. https://twitter.com/devongovett/status/1602459843784282113 This is something we deal with everywhere which is why I focus on this issue since it's treated so haphazardly in three, and only previously due to lack of observability or tooling. This would otherwise be automated (via an ES version, browserslist) if addons weren't shipped as source code, which I believe is the main issue here; this is automatic for the core and there's still a myriad of ways of doing this for addons with a policy in place, such as the linter or ideally automated as a bundle. |
|
(Some) addons already use class fields, so they already don't support Webpack 4, but nobody has complained, so maybe we still can use class fields in addons. |
|
|
Then if somebody wants to use addons witn Webpack 4 they can use |
|
What I was saying is that the comment is wrong and irresponsible. That project exists because of the mismanagement of addons which I've been trying to resolve here by means of tooling, discussion, or code change. It doesn't matter whether you want to support Webpack 4 as much that this is codified in the first place. Doing this after the fact by means of damage control is not okay. |
|
Do you know if this would still breaks bundlephobia? And, if so... maybe we can just add a transpilation step for the builds to avoid breaking it? |
@mrdoob apparently it was fixed. Did a quick test and it works fine! |
|
Alright! Lets give it a try. |
But maybe we should still add a transpiler to account for the usage of Webpack 4? |
|
I rather wait to see how many people complain. |
|
However, all builds except |
|
Can we also enable transpilation for |
|
If we did, when do you think we should remove it? |
|
When the number of downloads of Webpack 4 per week will become significanfly less than that of Webpack 5. |
|
Ah... Where can I see that info? 👀 |
|
Here: https://www.npmjs.com/package/webpack?activeTab=versions. Currently the number of Webpack 4 downloads is more than twice of Webpack 5's. |
transpilation is a necessary step, there's no incentive to remove it. babel grows with javascript and incorporates all new specs and features, then makes sure the code can run everywhere. threes current setup says browserslist > 1%, we can exclude really old browsers, modern only, and it specifically marks class props for transpile. not doing that for addons (and/or three.module) will bar people from using this library. but why? it would just be an additional rollup entry without any downsides. 🤷♂️ |
That is unfortunately not true. We always have tried to keep the number of dev dependencies to a minimum for easier maintenance. One major reason for With canisue and listings like #24237 (comment) (@marcofugaro thanks again!), I think we can estimate when is the right time to introduce new JS language features. Sidenote: Personally, I believe transpilation is an awful thing in the web context. JavaScript is easy to use and learn because you write code that is 1:1 executed in the browser. That is what the web makes exciting. When we use language features that common browsers and build tools can understand, we don't have to give up this great feature. Of course we should also encourage users and devs to upgrade their browsers and tools as far as possible. |
|
But if we are going to use a feature that less than a third of Webpack users can work with, isn't it more of breaking their tools rather than encouraging an upgrade? |
|
Yes, I think the introduction of class properties comes too early. I don't think there is an urgent reason to rely on this syntax right now. Same for stuff like nullish coalescing. |
|
@Mugen87 i think there some major misconceptions ...
shipping code that only few browsers can handle is making it harder for any individual or professional entity to publish software. and this all seems more ideologically motivated rather than objective. this is an open source library, people who want to study source come here. out there the only thing this library has to accomplish is to be used.
this is what babel is for, and it is excellent at it. and you are using it already for that exact purpose, but only for that file, and not for another.
a build process is there to avoid complexity. not having it would only delegate the complexity towards end users, as it already does, and oss contributors with arbitrary rules like "you can't use nullish coalescing just ...yet". |
|
The current proposals are:
Both choices are fine, and there are downsides to each, including Babel1 and ESBuild. Either way, the output is ES2018, and the difference is only in maintainer-facing tools. The decision on class properties appears to be made: we are not using them at this time. (#25168) I don't think class properties are going be a winning argument for transpiling the whole codebase, they don't matter to maintainers that much; this isn't a good context to rehash it again. If anyone would like to make a broader proposal about improving 1 predictable performance in hot code paths, predictable size, not mucking with regenerator-runtime dependencies, ... |
|
It's funny how the problem that transpilers were supposed to solve is now worse because of... transpilers. We've reverted this change (#25168). We can give it another go next year. |
How can we update the linter so this is automatically checked? When writing #25172, I've used the search to find the respective syntax. |
This doesn't affect us and is why we choose es2018, either handwritten or via transpiler. ESBuild has a good breakdown of syntax-to-version if you're unsure of what this changes: https://esbuild.github.io/content-types/#javascript.
I added notes and am tracking it in #25185. |
Related issue: #22437
Description
1 year has passed since last time we tried. Maybe it's time to test the waters again? 🤓
create-react-app has since released version 5, which uses Webpack 5. So it doesn't error there anymore.