Linting: Update linter to enforce ES 2018 features

[epreston]

Fixed #25185.

Description

Update linter to enforce ES 2018 features.

This has four changes:

• add setting to configure linter to enforce 2018 features
• separate config for linting into config file.
• update the 3 header imports that were using 2020 features
• to maintain the same public interface, add export objects MathUtils, AnimationUtils, and DataUtils

The setting that does the work of enforcing 2018 features is:

  "parserOptions": {
    "ecmaVersion": 2018,
    "sourceType": "module"
  },

The 'module' setting has been provided by the custom style plugin, it is duplicated here for clarity. This also ensures that the style plugin is only providing styles rules.

This also overrides the environment setting from the custom plugin to ensure only 2018 globals are defined. Browser and node settings are duplicated for clarity. This also ensures that the style plugin is only providing styles rules.

  "env": {
    "browser": true,
    "node": true,
    "es2018": true
  },

The lines in 'Three.js' using 2020 syntax were:

export * as AnimationUtils from './animation/AnimationUtils.js';
export * as MathUtils from './math/MathUtils.js';
export * as DataUtils from './extras/DataUtils.js';

With an export object added, these become:

export { AnimationUtils } from './animation/AnimationUtils.js';
export { MathUtils } from './math/MathUtils.js';
export { DataUtils } from './extras/DataUtils.js';

If the project wants to remove a dependency, we can copy the 15 lines of style settings from the plugin into the lint config file.

[epreston]

MathUtils, AnimationUtils, and DataUtils need to be updated to export their namespaces.

[epreston]

If the project wants to remove a dependency, copy the 15 lines of style settings into the lint config file.

[epreston]

Anyone worried about tree-shaking:

As a confirmation take a look at what we ship now using the 2020 syntax. I'm going to use the middle size one as an example to reduce the reading.

The old 2020 line:

export * as AnimationUtils from './animation/AnimationUtils.js';

Is currently being translated to:

var AnimationUtils = /*#__PURE__*/Object.freeze({
	__proto__: null,
	arraySlice: arraySlice,
	convertArray: convertArray,
	flattenJSON: flattenJSON,
	getKeyframeOrder: getKeyframeOrder,
	isTypedArray: isTypedArray,
	makeClipAdditive: makeClipAdditive,
	sortedArray: sortedArray,
	subclip: subclip
});

This has been updated to:

const AnimationUtils = {
	arraySlice: arraySlice,
	convertArray: convertArray,
	isTypedArray: isTypedArray,
	getKeyframeOrder: getKeyframeOrder,
	sortedArray: sortedArray,
	flattenJSON: flattenJSON,
	subclip: subclip,
	makeClipAdditive: makeClipAdditive
};

It gets translated to:

const AnimationUtils = {
	arraySlice: arraySlice,
	convertArray: convertArray,
	isTypedArray: isTypedArray,
	getKeyframeOrder: getKeyframeOrder,
	sortedArray: sortedArray,
	flattenJSON: flattenJSON,
	subclip: subclip,
	makeClipAdditive: makeClipAdditive
};

Keeping it clean, the final bundler (client application) works its magics. Its possible we get slightly better tree-shake using the 2018 syntax. Keep in mind this is an isolated context, relating to a library (not client application), and a very specific import command.

In the old code, final bundler can only remove the whole object if its entirely unused, which will be created in any case (new or old code). Tools can remove constants if they are entirely unused, so we have even ground.

[epreston]

For evaluation, a test rig of the different builds and tree shake effects can be found here:

https://github.com/epreston/three-treeshake-coverage

Comment and uncomment the different imports, build and inspect the results in the dist folder.

In all cases, AnimationUtils is correctly tree-shaken.

[donmccurdy]

Is it possible to configure the linter to enforce ES2018 features, but to ignore ES2020 import/export syntax? I don't think there is any problem with shipping those ES2020 exports, and might allow us to make a narrower change here.

If that's not possible, that's OK, but I didn't see that mentioned in the thread so far.

[epreston]

Anything's possible with configuration and tooling. We have great ideas for partial solutions going 2020 with restrictions. I don't see a built in way to approach the problem from that direction (2018 with 2020 imports).

In all cases, we never ship these exports. The code this replaces is a developer convenience to shave 20 lines off the code we maintain. The code it generates is equal. What we trade is complexity, validation of PRs, and standards with partial solutions.

[donmccurdy]

I see – thanks! No concerns about the exports in that case, changing them to reflect how they already get shipped in the bundle sounds good.

[marcofugaro]

Yes this PR does not affect tree-shaking, nor does it make it better.

My point is that this PR changes the exports in a confusing way for no apparent reason, since the import/exports get compiled away. I don't see how this change is beneficial for future maintainers to understand what is going on. The real issue is with eslint, a tool shouldn't dictate how we structure our code.

But I guess this can be overlooked one since it's supposed to be temporary.

[donmccurdy]

We want to have automated warnings when our source code is not ES2018 compliant. I would've ideally excluded imports/exports from that, since they're handled by the build process anyway, but that path doesn't seem to be practical, so I'm fine with changing the imports/exports this way for now. We can clean them up later if needed.

It's not just about 1-2 unsupported ES2020 features — we want our code to be ES2018 compliant, and we don't want to be surprised by a bit of newer syntax sneaking in, or surprised by transpilers doing something we didn't expect. I worry that just enforcing ES2020 and adding custom rules on top will not give us those guarantees, is that wrong?

[epreston]

I'm confused. Changing the only 3 imports that are using a different syntax so they are the same as other 100 adds confusion ? The lines that created confusion about tree-shaking ?

The project agrees on a syntax level, they is set in one line, PR's get validated against it, no gaps. You set any tool to 2018 and you get the same results. Seems kinda clean having it configured according to the manual in the most typical way.

[epreston]

we don't want to be surprised by a bit of newer syntax sneaking in

Please don't look at line 1891 of examples/GLTFExporter.js until I submit a PR.

[marcofugaro]

Hmm yeah I guess there is no way of having full ES2018 coverage without changing the import as well.

[mrdoob]

Ready to merge? 🧐

[marcofugaro]

Go ahead!

[epreston]

@mrdoob can we wrap this up ? I'd like to move on to the next.