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

Convert Examples folder to ES6 Modules #14803

Closed
wants to merge 10 commits into from

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Aug 29, 2018

Hello! This PR converts most of the example code in the examples/js directory and all of the example HTML files (_module appended) to use ES6 modules. This (for the most part) addresses #9562. There's still a bit left to do but I wanted to get a PR going to get feedback and a discussion going before going too far down this path. I'm hoping this can provide a significant start and pattern for migrating the examples folder to use modules.

You can see the original and module versions of the THREE.js examples here: https://rawgit.com/gkjohnson/three.js/examples-to-imports/examples/index.html

All converted ES6 module files have been added into the examples/modules folder in the same structure they can be found in the examples/js directory. The module versions are then converted to UMD modules using Rollup in a way that enables backwards compatibility.

It's also worth noting that this PR includes the changes from #14795, as well.

What it looks like

You can see what the STLExporter looked like before this PR, as an ES6 module, and after being converted to UMD.

Here's a link to the modules folder for easier file browsing.

How

I broke it down a bit more in this repo with the conversion script but the gist of it is that by scraping the example files for anything that sets a new variable on THREE you can determine what a script is "exporting". This means that only example scripts that are extending the THREE object could be converted. I can write up a more robust description if people are interested. The script itself has admittedly grown pretty messy.

Backwards compatibility

I think it's preferrable to maintain the examples as modules going forward so I've added a rollup config that converts all the ES6 modules into UMD modules that extend the THREE object. That means all of the current examples work as-is with no changes (with the exception of OBJ2Loader described below). The built UMD modules could be minified to discourage editing of those files directly.

This config basically only works for modules that we want to work in this backwards compatible way, though. If new example modules are added and the intent is to not extend THREE when generating the UMD file, then we'll have to find another approach. Maintaining a whitelist of modules to convert this way would be one option.

The build can be run with npm run build-examples.

What was converted

The full list of converted example files can be found here and the list of unconverted files here.

Why wasn't X converted?

  • The libs directory was deliberately ignored because they're external libraries.
  • The nodes directory is already written using modules (but still relies on global THREE) so it was ignored.
  • The loaders/sea3d because the relationship between files was harder to extract than the others in part because they extend a THREE subobject.
  • XLoader.js wasn't converted because it's already in a UMD wrapper which was causing problems for conversion.
  • For the most part everything else wasn't converted because it didn't use or depend on other generated modules or THREE.

Why doesn't X / module example page work?

The list of which examples aren't working with modules are outlined here.

Most of the examples that don't work are having problems because they depend on one of the unconverted files above. Others are failing because of the stricter environment that modules impose (no implicitly accessing window, setting members on an imported object, etc).

The couple exceptions are the OBJ2Loader, which isn't working in part because it generates code (which I'll have to track down a bit further or manually fix) and the BokehPass, which imports the wrong BokehShader (there are two BokehShaders).

What Next

If we take this approach I think the remaining examples can be moved into the modules folder and converted by hand as desired. The examples that don't work with modules can be fixed, as well. I could make an issue for tracking the remaining items to be converted to imports. We'll also have to understand how different types of modules are converted to UMD with rollup (as mentioned above).

Thanks for taking a look! And let me know your thoughts!


edit

Suggestions

Suggestions and other thoughts to keep track of

  • Inject an ES6 module polyfill into module examples so they function in older browsers.
  • Examples build happens along with three.js core build
  • Minify the UMD modules
  • Use shortest paths in import statements

@donmccurdy
Copy link
Collaborator

donmccurdy commented Aug 29, 2018

Thanks for doing this! I have a similar PR (#13544) but yours is much more complete, so I'm happy to focus on this.

tl;dr generally I think this is the right approach to take, comments below are just meant to identify particular issues/concerns with details of the implementation.

(1) dependency resolution

Taking GLTFLoader as a starting point, because I'm familiar with it:

import * as THREE from '../../../build/three.module.js';
import { DDSLoader } from '../../modules/loaders/DDSLoader.js';
import { BufferGeometryUtils } from '../../modules/utils/BufferGeometryUtils.js';

That first line is the big question for me — what does a user's build system do when it sees:

import { WebGLRenderer, Scene } from 'three';
import { GLTFLoader } from 'three/modules/loaders/GLTFLoader.js';

Does that create duplicate dependencies on WebGLRenderer and Scene, because GLTFLoader references them relative to itself rather than via three? Or are Webpack, Browserify, and Rollup able to de-dup that? Does * as THREE break tree-shaking? Lack of tree-shaking shouldn't block this PR (we can resolve it example-by-example) but duplicate dependencies would be a problem.

(2) UMD modules in repo

I'm inclined to say we don't need UMD modules in the three.js repository. Once we have ES6 modules in the release, we can put download links in the docs to something like https://bundle.run/[email protected]/examples/js/loaders/GLTFLoader.js, and have UMD modules generated dynamically + cached.

(3) polyfill for IE11

It looks like this PR would drop support for IE11 in our examples. Would just adding an ES6 modules polyfill bring that support back?

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Aug 29, 2018

I didn't see that PR! That other issue was getting so long it was hard to trace what efforts were currently ongoing. Regarding your questions:

(1) dependency resolution

what does a user's build system do when it sees...

The line import { WebGLRenderer, Scene } from 'three'; should also resolve to .../build/three.module.js because that's what the package.json points to so it should be possible to dedupe that file on build. My impression is that build systems are smart enough to handle that but maybe this is a corner case?

At the moment the code in src isn't usable without custom build steps (#14446) but if that ever gets resolved then both the package.json and example modules can be updated to point at the files directly in the src directory.

Another note is that just referencing a module by package name means that it cannot directly load in a browser, so just importing from 'three' doesn't seem workable to me.

(2) UMD modules in repo

I'm inclined to say we don't need UMD modules in the three.js repository....

I included them purely for backwards compatibility. I think they should at least be marked as deprecated for a couple releases before being removed entirely as often happens with other breaking changes in THREE. We can add a warning log in the UMD banner code to notify people of the change. I think a lot of people (myself included) use the code in examples as though it were a part of the main library despite the folder name (a discussion for another time 😊)so removing them outright would break projects.

@WestLangley also expressed interest in another PR in maintaining backwards compatibility in examples for the sake of jsfiddles and other direct links to the repo here, so I think there's a discussion to be had. I'd be interested in other opinions here, as well!

(3) polyfill for IE11

Would just adding an ES6 modules polyfill bring that support back?

I actually didn't know there was a standard ES6 module polyfill -- that sounds like a pretty good option, though! I wasn't necessarily imagining that the module examples would replace the current example pages for exactly the reason of browser support, but it does make sense to keep the whole repo moving forward if we can. I primarily built the example html pages for the sake of testing.

Thanks for the feedback!

@moroine
Copy link
Contributor

moroine commented Aug 30, 2018

About the compatibility, I've try some time ago to add ES6 import polyfill: https://rawgit.com/moroine/three.js/poc-example-es6/examples/misc_controls_orbit.html

Also considering the compatibility table of module we could also just provide examples for those browsers and eventually a nomodule build for use on master branch (to keep the official examples compatible with old browsers).

About the UMD version, I didn't notice that issue. But maybe those UMD files could be generated on build only for master branch ?

@gkjohnson
Copy link
Collaborator Author

@moroine

Also considering the compatibility table of module we could also just provide examples for those browsers and eventually a nomodule build for use on master branch (to keep the official examples compatible with old browsers).

If the polyfill really works that simply then it seems like a great solution and ensures we don't have deal with building the html examples, as well, while still supporting old browsers.

About the UMD version, I didn't notice that issue. But maybe those UMD files could be generated on build only for master branch ?

What issue are you referring to? I was imagining that the examples modules would be built at the same time as the build for THREE core in builds to ensure no added burden on maintainers or contributors. Is master where that's done?

@sompylasar
Copy link

RE: module deduping in a bundler, I can say for Node and webpack, it should be fine: whatever you put into require or import is called a module request which is first resolved to the full path to the actual file, and this full path is used as the key into the module cache, so all module requests that resolve to the same file will use the same module object.

@Itee
Copy link
Contributor

Itee commented Sep 12, 2018

@donmccurdy Is there any chance that this PR will be merge ?

@donmccurdy
Copy link
Collaborator

Ultimately that's a question for @mrdoob. From previous comments (#9562 (comment), #9562 (comment)) I think we do agree that we want to make examples/js/* available as ES6 modules. But there are open questions on how to do that.

@mrdoob this may be a question only you can answer — In the long term (after all modern browsers support ES6 modules, and after any transition/migration period) what is the ideal state for the project? All examples use individual ES6 modules? One big three.examples.module.js bundle? Keep both ES6 and UMD in the repo?

I think it will be easier to agree on a transitional path if we have the "end state" agreed upon.

@gkjohnson
Copy link
Collaborator Author

FWIW I favor keeping the modules and umd files in separate folders instead of having them side-by-side. I think it keeps the folders more organized and clear. And while the monolithic three.examples.module.js might be a nice convenience I think the example modules should be available individually and maintained as modules moving forward.

Once there's consensus on some of these open questions (including #14795) I'll make any required changes and rebuild the modules. If there's anything I can do to make this PR easier to understand or look at @mrdoob, let me know!

@Itee
Copy link
Contributor

Itee commented Sep 12, 2018

@gkjohnson I don't know if you're aware about three-full but all examples files are currently converted as es6 modules. We have the capability to redirect them in core during convert process too. Maybe this could help you in some way.

I think the example modules should be available individually and maintained as modules moving forward.

There is a lot of proposition about what we should do about examples. But keeping them as "external" modules doesn't sound good to me. Lot of them should be move into the core.

Anyway, if @mrdoob think to merge it and if you need some help for this PR, please tell me.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Sep 12, 2018

@Itee Thanks! I hadn't seen that but I'll take a deeper look when I get a chance.

There is a lot of proposition about what we should do about examples. But keeping them as "external" modules doesn't sound good to me. Lot of them should be move into the core.

I understand but I think that's a very different discussion to this PR. For me the point of this PR is converting the examples such that they're directly loadable in a browser using a modern es6 import syntax while still supporting backwards compatible use. I worry that if this discussion balloons into suggestions about what should or shouldn't live in the examples folder then progress won't be made. I just want to keep this well-scoped. I agree with the general sentiment though and would gladly discuss this in another issue!

@sompylasar
Copy link

@gkjohnson

I understand but I think that's a very different discussion to this PR. For me the point of this PR is how to convert the examples such that they're directly loadable in a browser using a modern es6 import syntax while still supporting backwards compatible use. I worry that if this discussion balloons into suggestions about what should or shouldn't live in the examples folder then progress won't be made.

But the point of the original issue this PR is supposed to solve is to make whatever reusable modules linger in examples available for import into ES6 code (either bundled or browser-supported) and used via named imports, not via global THREE variable, while keeping backwards compatibility of directly loading them into the examples HTML demo pages without a build step.

@donmccurdy
Copy link
Collaborator

But keeping them as "external" modules doesn't sound good to me. Lot of them should be move into the core.

Please consider this a separate issue/request from making examples into ES6 modules. Discussing it here will only complicate progress on this PR, where I think we have more consensus.

@gkjohnson
Copy link
Collaborator Author

@sompylasar I think we're saying the same thing. The transformed modules do not rely on a global THREE definition. The es6 modules are transformed to UMD modules that extend the global THREE object the way they always have so existing projects don't have to change, though. No build process is required for the end user or the html demo files. Sorry for the miscommunication!

@Itee
Copy link
Contributor

Itee commented Sep 13, 2018

Please consider this a separate issue/request from making examples into ES6 modules. Discussing it here will only complicate progress on this PR, where I think we have more consensus.

Yes, i agreed.

But the point of the original issue this PR is supposed to solve is to make whatever reusable modules linger in examples available for import into ES6 code

I agreed, too. But...

This PR try to convert examples as UMD module in view to be usable in browser and (still supporting backwards compatible use). So we can consider this PR as an intermediary step that improve current state without change the fundamental problem about es6 convertion.

@gkjohnson I'm currently refactoring example in the Three class style. That will allow you to perform a better and easier parsing base on the same rules for all of them. I won't that enter in conflict with this one, hopefully.

@Methuselah96
Copy link
Contributor

Methuselah96 commented Oct 18, 2018

What's the status of this PR? Is this PR waiting on anything (besides the conflicts being resolved)?

@gkjohnson
Copy link
Collaborator Author

Hey @Methuselah96 sorry for taking a few days.

I'd really like to work on this more but I'm waiting on feedback and direction for @mrdoob on where this should go and whether this is an acceptable approach to convert the examples. If there's interest or changes needed I'll gladly put the time in to move the PR in that direction and complete some of the other items in the "suggestions" list above.

@riyuedk
Copy link

riyuedk commented Nov 16, 2018

Error in data(): "TypeError: Cannot read property 'position' of undefined" in examples/modules/controls/OrbitControls.js
why?

@Methuselah96
Copy link
Contributor

@mrdoob Can you comment on whether this is an acceptable approach to convert the examples? It seems like this would be a good start to addressing a two-year-old issue that appears to be causing problems for many people.

@mrdoob
Copy link
Owner

mrdoob commented Jan 3, 2019

Some progress... #15518

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 19, 2019

Closing since most of the js files and demos in the examples are module-based now (see #16688). Thanks though for the initiative 👍 !

@Mugen87 Mugen87 closed this Jun 19, 2019
@mrdoob
Copy link
Owner

mrdoob commented Jun 19, 2019

Thanks @gkjohnson! 🙏

@gkjohnson gkjohnson deleted the examples-to-imports branch November 1, 2019 17:04
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.

9 participants