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

Eslint: add support for class properties #21259

Merged
merged 1 commit into from
Feb 16, 2021

Conversation

marcofugaro
Copy link
Contributor

Related issue: #20009 (comment) (remake of #21238)

Description

Eslint also needs to know about babel to support class properties.
In this PR I added @babel/eslint-parser as a parser for eslint. This does not change the current eslint bahaviour.

Third time the charm?

@DefinitelyMaybe
Copy link
Contributor

Third time the charm?

tehe! I been there.

@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2021

Third time the charm?

🤞

@mrdoob mrdoob added this to the r126 milestone Feb 16, 2021
@mrdoob mrdoob merged commit 8cdc5e3 into mrdoob:dev Feb 16, 2021
@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2021

Seems to be working! Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2021

Unfortunately I have to revert this. Safari doesn't support class properties yet so all examples are currently broken.

@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2021

e7ba155

@marcofugaro
Copy link
Contributor Author

marcofugaro commented Feb 23, 2021

@mrdoob okay 👍 we'll have to wait then.

We could also choose to transpile build/three.module.js as well, and remove the transpilation step when they will be supported.
BTW now that IE support is dropped, do we still need to transpile three.js and three.min.js? Classes and other es6 features are supported in all modern browsers.

@mrdoob
Copy link
Owner

mrdoob commented Feb 23, 2021

BTW now that IE support is dropped, do we still need to transpile three.js and three.min.js? Classes and other es6 features are supported in all modern browsers.

I think I'll wait until importmaps land in browsers.

@marcofugaro
Copy link
Contributor Author

I think I'll wait until importmaps land in browsers.

We are not using any feature related to import maps in the src/ though, right? 🤔

@mrdoob
Copy link
Owner

mrdoob commented Feb 23, 2021

No we are not. But the development experience using three.module.js right now is... problematic.
So until importmaps lands in browser doesn't feel right to force people to move to modules.

@marcofugaro
Copy link
Contributor Author

marcofugaro commented Feb 23, 2021

I think you misunderstood 🙂 I meant removing the babel transpilation step for three.js and three.min.js.

I am talking about these steps:

babel( {
babelHelpers: 'bundled',
babelrc: false,
...babelrc
} ),
babelCleanup(),

The transpilation is just for modern es features (classes, let/const, arrow functions), the conversion from es modules to single umd file that users can use with <script src="three.js"></script> is done by rollup.

@mrdoob
Copy link
Owner

mrdoob commented Feb 23, 2021

I misunderstood indeed!

We do still support IE11 in this example:
http://raw.githack.com/mrdoob/three.js/dev/examples/misc_legacy.html

And the current babel setup doesn't seem to cause problems so I see no reason for removing it.

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.

3 participants