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

JSM: Add Canonical Modules White List #15599

Closed
wants to merge 9 commits into from

Conversation

gkjohnson
Copy link
Collaborator

As discussed here and here.

Adds a script to determine which example modules should be considered the canonical source. If it is considered the canonical source then a "do not edit" banner is added when generating a UMD module. If it is not then a "do not edit" banner is added when generating the jsm files.

The white list can consist of strings or regex. Here's an example banner:

/**
 * Generated from original source in "examples/js/controls/MapControls.js".
 * Not intended for editing.
 */

… a module should be considered the canonical source
@donmccurdy donmccurdy changed the title Add Canonical Modules White List JSM: Add Canonical Modules White List Feb 15, 2019
@gkjohnson
Copy link
Collaborator Author

@mrdoob
While you're looking at module PRs what are you thoughts on this?

This PR provides a way to consider some of the JSM scripts as canonical and rely on the rollup script to convert examples to UMD as opposed to relying on the modularize script.

There have been a few PRs that have converted modules by hand (#15899, #15656) that can't be merged because they'll become out of date if the existing js scripts get updated. Other scripts such as EffectComposer.js will require hand changes that like separating EffectComposer and Pass into separate files.

I think it'll be important to have some process by which we consider the module versions of the example canonical as more get converted.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 15, 2019

I agree with creating a whitelist, so that some files can be graduated to JSM-as-source. But I'm not necessarily sure whether loaders/* are all ready to be on that whitelist yet.

Other scripts such as EffectComposer.js will require hand changes that like separating EffectComposer and Pass into separate files.

These don't necessarily need to be separate files right, they could just have multiple exports? I'm tempted to argue an even more aggressive conversion for postprocessing/*, nodes/* and maybe other folders: the new ES modules in these folders are distinct files and may depend on one another, with a new entrypoint like postprocessing/index.js. The generated UMD module can be just a single file, e.g. postprocessing.js. That's a breaking change for users doing individual CommonJS-ish imports from these files, but given that they have to do CommonJS imports writing everything to the global namespace in a very specific order, I think it's probably worth it.

import { EffectComposer, Pass, BloomPass } from 'three/examples/jsm/postprocessing/';

const { EffectComposer, Pass, BloomPass } = require('three/examples/js/postprocessing.js');

@gkjohnson
Copy link
Collaborator Author

These don't necessarily need to be separate files right, they could just have multiple exports?

Without refactoring EffectComposer.js that's not the case. I discussed it a bit in #15832 but EffectComposer.js currently defines EffectComposer and Pass. It also relies on ShaderPass.js, which relies on Pass from EffectComposer.js. So a circular dependency is formed from EffectComposer.js depending on ShaderPass.js which depends on EffectComposer.js. I'm not sure of any way to enable a circular dependency like that with modules so my first inclination was to separate Pass into a separate file but maybe there's another way.

I'm tempted to argue an even more aggressive conversion for postprocessing/*, nodes/* and maybe other folders

I don't really have strong feelings on this other than that it seems unnecessary to include a bunch of postprocessing pass code if it isn't being used and it's not backwards compatible, which has been mentioned as a desire previously. It is really inconvenient to have to include all the global scripts, though...

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 15, 2019

I'm not sure of any way to enable a circular dependency like that...

Ah, ok yes we should split the file then.

I don't really have strong feelings on this other than that it seems unnecessary to include a bunch of postprocessing pass code if it isn't being used and it's not backwards compatible, which has been mentioned as a desire previously. It is really inconvenient to have to include all the global scripts, though...

How is Rollup going to create separate UMD files for ES modules that depend on one another? If it's easy enough get have it convert back Pass back to THREE.Pass I guess that's fine, but it sounds complex and I would rather not have regular expressions on the ES->UMD side of the conversion process.

In general these folders are things that would be separate NPM packages (@threejs/postprocessing), if managing multiple NPM packages in one repository weren't a nightmare, and in that spirit I think it's reasonable to ask users to support ES modules if they care about tree-shaking. Getting the order of imports right for postprocessing is a headache, in its current form.

So yes, I'm suggesting we break back-compat for these specific cases (postprocessing, nodes, curves), but won't complain if others disagree.

@gkjohnson
Copy link
Collaborator Author

How is Rollup going to create separate UMD files for ES modules that depend on one another

It's actually not so bad and my previous module PR #14803 supported it by just marking every dependency as external in the rollup config which means the produced package will look on the global field (THREE in this case) for the class.

I'll save the npm packages can of worms for another discussion but I've been operating under the assumption backwards compatibility is important when converting and if that's true then Rollup can support that when going from es6 -> umd. If that's not needed then even better!

@donmccurdy
Copy link
Collaborator

@gkjohnson what do you think of modifying this so it doesn't have an explicit whitelist, but instead:

  • rollup-examples.config.js contains a whitelist list of files like modularize.js
  • both rollup-examples.config.js and modularize.js always add a banner to their output

I guess I'm looking for the simplest possible way to start converting a folder like postprocessing/*, where files have dependencies on one another. I think I'll have to do that by hand, and would like to do that only once.

@gkjohnson
Copy link
Collaborator Author

Yeah I'm happy to simplify this however makes sense -- there is the caveat that there will have to be care taken to ensure that the modularize and rollup lists are mutually exclusive, otherwise we'll wind up with scripts overwriting each other. I can look into this a bit more either tonight or tomorrow.

I guess I'm looking for the simplest possible way to start converting a folder like postprocessing/*, where files have dependencies on one another. I think I'll have to do that by hand, and would like to do that only once.

Using either script from #14803 or #15832 could be a good starting point for this, but it's probably not so bad to do it by hand. And just for context I made PR #14795 awhile ago to address the cyclic dependency in the current EffectComposer to avoid this but it didn't gain much traction. After that change I was able to successfully convert the post processing files.

Do you intend for the rollup-converted modules to be backwards compatible after the conversion? If so that will mean there have to be special settings for EffectComposer.js rollup config that tell it to include the Pass script in the file instead of treating it as external. If so I can include that capability in my changes.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 26, 2019

Using either script from #14803 or #15832 could be a good starting point for this, but it's probably not so bad to do it by hand.

The conversion is easy enough, but unless it's 100% automated (and I don't think either PR above is there yet) I don't want to do it without a way to generate the js/ version from the jsm/. Moreover, I don't think we can realistically create a script that converts every file in js/ to a functional ES module. We need a way to do hand conversions to jsm/ for the special cases, and to generate js/ from that.

Basically the rule should be for all files, either jsm/ can be generated automatically from js/, or vice versa, and we should never be in a position where a change to one requires manual changes to the other.

Do you intend for the rollup-converted modules to be backwards compatible after the conversion? If so that will mean there have to be special settings for EffectComposer.js ...

I don't think that's a good idea or maintainable, personally. We should generate output that is consistent with Rollup best practice.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Mar 26, 2019

Basically the rule should be for all files, either jsm/ can be generated automatically from js/, or vice versa, and we should never be in a position where a change to one requires manual changes to the other.

Totally agree. I didn't intend to say that we should rely on the script entirely or that they wouldn't need hand fixes afterward. Just that they might serve as a good start to generating a module.

I don't think that's a good idea or maintainable, personally. We should generate output that is consistent with Rollup best practice.

I'd like to keep things simple, too, but keep it in mind in case backwards compatibility winds up being a concern. The config would look something like this:

var files = [{
  path: 'jsm/postprocessing/EffectComposer.js',
  include: ['jsm/postprocessing/Pass.js']
}];

What are you envisioning the converted files looking like after rollup is done with them? Will all postprocessing effects be separate UMD files as they are now? Or are you planning to create an index file so they will all be bundled into a single UMD file that encompasses all post processing effects?

Edit: Just to note that the rollup script as it is right now will create a new UMD file for every module file processed.

@donmccurdy
Copy link
Collaborator

What are you envisioning the converted files looking like after rollup is done with them? Will all postprocessing effects be separate UMD files as they are now? Or are you planning to create an index file so they will all be bundled into a single UMD file that encompasses all post processing effects?

I'm fine with either, but I think I value a simple conversion and final result more than backward-compatibility in this case.

@gkjohnson
Copy link
Collaborator Author

@donmccurdy I've updated the rollup example config to be modeled a little more closely to modularize.js and removed the isModuleCanonical.js list. Both scripts now use an internal list of files to determine what to convert and each adds a banner to the top of the converted file.

The big difference is that the rollup config specifies a regex for the file path while modularize.js specifies the specific file as a string. I'm happy to change this but it seemed convenient to be able to convert whole folders. At the moment every file is converted 1-to-1 so you'll get a new UMD js file for every jsm file.

var dstFolder = __dirname + '/examples/js/';

var files = [
// { path: /postprocessing[\/\\].+\.js$/ },
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No files are converted at the moment (so rollup will throw an error) but they can be added in future PRs. You can use rollup to convert the files in the jsm/controls folder by changing this to:

var files = [
	{ path: /controls[\/\\].+\.js$/ }
];

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 27, 2019

The changes look good to me, thanks! The regex should be fine.

I'd like to test this on some files (maybe postprocessing/ or curves/?) just to confirm. Assuming everything works, @mrdoob would you be OK with merging some PRs that put converted files on the jsm->js list, rather than the js->jsm list?

@gkjohnson
Copy link
Collaborator Author

Closing in favor of #16920.

@gkjohnson gkjohnson closed this Jun 27, 2019
@gkjohnson gkjohnson deleted the canonical-modules branch September 16, 2021 15:51
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.

2 participants