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

Examples: Deprecate examples/js. #18749

Merged
merged 2 commits into from
May 27, 2020
Merged

Examples: Deprecate examples/js. #18749

merged 2 commits into from
May 27, 2020

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Feb 26, 2020

This PR will deprecate all files in the examples/js directory except for all external libraries. A console message will warn the user about the removal of those files in one of the upcoming releases. Right now, I've added R117. The exact date is of course open for discussion. Same for the warning's text.

The PR replaces #17544.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 26, 2020

I've written this script to perform the addition of the console log in the js files:

const fs = require( 'fs' );
const path = require( 'path' );

const srcFolder = __dirname + '/../examples/js';
const excludedDirectories = [ 'libs', 'vr' ];

function walk( dir ) {

	let results = [];
	const list = fs.readdirSync( dir );
	list.forEach( function ( file ) {

		if ( excludedDirectories.includes( file ) === true ) return [];

		file = dir + '/' + file;
		const stat = fs.statSync( file );
		if ( stat && stat.isDirectory() ) {

			results = results.concat( walk( file ) );

		} else {

			results.push( file );

		}

	} );
	return results;

}

const files = walk( srcFolder );

for ( let file of files ) {

	const data = fs.readFileSync( file );
	const fd = fs.openSync( file, 'w+' );

	const filename = path.parse( file ).name;
	const buffer = Buffer.from( `console.warn( "THREE.${filename}: Importing from 'examples/js' is now deprecated. Please import ES6 modules from 'examples/jsm' instead. Refer to the announcement and documentation, or reach out on the forums if you need guidance with this change. Announcement: TODO. Documentation: https://threejs.org/docs/index.html#manual/en/introduction/Import-via-modules." );\n`, 'utf8' );

	fs.writeSync( fd, buffer, 0, buffer.length, 0 );
	fs.writeSync( fd, data, 0, data.length, buffer.length );
	fs.closeSync( fd );

}

It's no part of the PR hence I share the code here.

@WestLangley
Copy link
Collaborator

This file is deprecated and will be removed with R117

Maybe refer to it as "release r117"

This file is deprecated and will be removed with release r117"

so as to follow the three.js nomenclature: https://github.com/mrdoob/three.js/releases/tag/r113.

@WestLangley
Copy link
Collaborator

This is going to break a lot of SO answers.

That is fine with me, but I am trying to maintain 1300+ of my SO answers, and like others, there will be a lot of fiddles that will no longer work.

I do not want to modularize each of those fiddles, so I guess I will have to delete them.

Since this will also impact others who have provided answers or blog posts online, perhaps there are suggestions on how to proceed in this regard...

I do not think it is a good idea to link to outdated, static revisions of the library in SO answers.

@donmccurdy
Copy link
Collaborator

I would suggest softening the warning a bit. It will be the first that many users have heard of the deprecation, and this will be the most significant breaking change our API has had in a while. I agree that there's good reason for it, but even for experienced developers, upgrading an application's build process to support ES6 does take a bit of time. I'm not sure that I would include a deadline in the first release — we'll probably get a few "I can't/won't upgrade because ________" responses, and it'd be good to work through the most common issues before we start getting the more heated responses that could accompany a deadline.

THREE.${FILE_NAME}: Importing from 'examples/js' is now deprecated. Please import ES6
modules from 'examples/jsm' instead. Refer to the announcement and documentation below,
or reach out on the forums if you need guidance with this change.

• Announcement: https://discourse.threejs.org/${FORUM_LINK}
• Documentation: https://threejs.org/docs/index.html#manual/en/introduction/Import-via-modules

I also expect we'll need to provide more documentation than is currently available on the Import via modules page. Do we expect this warning would go out in r115? I can probably help write that, but would need to know the planned timing.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 27, 2020

@donmccurdy I've updated the warning with your suggestion. To avoid complexity, it would be good if the warning is a one liner. This makes it easier to remove it in modularize.js.

I would appreciate if you could write the announcement in the forum and enhance the Import via modules page a bit. Such things are not really my strength.

@WestLangley Yes, it's unfortunate when live examples break, however, it's also our own fault. Instead of using links like

https://threejs.org/examples/js/controls/OrbitControls.js

which will obviously not working anymore after the removal, we should have used links like:

https://cdn.jsdelivr.net/npm/three@0.113.2/examples/js/controls/OrbitControls.js

Of course CDN can also shut down but using a locked version is definitely better than always targeting the latest version.

@WestLangley
Copy link
Collaborator

using a locked version is definitely better than always targeting the latest version.

I disagree with that, which is why I did not do it that way. As I said, that means the SO fiddles will continue to work, but maybe not with the current release -- and that is not particularly helpful to users.

In any event, removing /js is going to be very disruptive -- on many levels.

@donmccurdy
Copy link
Collaborator

To avoid complexity, it would be good if the warning is a one liner.

Sorry to bikeshed, if this is a pain then please ignore my suggestion. But I think it could be done with something like:

console.warn( THREE.LEGACY_IMPORT.replace('{filename}', 'GLTFLoader') );

@donmccurdy
Copy link
Collaborator

I would appreciate if you could write the announcement in the forum and enhance the Import via modules page a bit.

Sure. Targeting r115 (for the announcement), I assume?

@looeee
Copy link
Collaborator

looeee commented Feb 29, 2020

That is fine with me, but I am trying to maintain 1300+ of my SO answers, and like others, there will be a lot of fiddles that will no longer work.
I do not want to modularize each of those fiddles, so I guess I will have to delete them.

It's totally understandable that you don't want to update all of these to modules. However, it would be a real shame to lose this huge bank of learning resources.

What if we reach out and ask for help updating them on the forum and on Twitter?

Do you have a list of all the examples somewhere? If so I'll make a post on the forum and see if there's any interest in doing this.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 29, 2020

Targeting r115 (for the announcement), I assume?

@mrdoob is this okay for you?

@Mugen87 Mugen87 marked this pull request as ready for review February 29, 2020 14:45
@hdtung88
Copy link

hdtung88 commented Mar 4, 2020

Hi everyone, I have an opinion about this problem as followings:

I think you guys shouldn't deprecate 'examples/js', because:
When I import three.js and its examples functions via modules, then obfuscate the whole code, webpack will obfuscate three.js too, this will makes the output js file size bigger => slower to load the application.
So normally I put all 'three.min.js' & others examples needed code in 'js' not 'jsm' & other libraries into a seperate file called 'libs.js'. and minify it. So webpack will have to obfuscate my main code only. This will reduce the total size of js codes alot.
Maybe there is someone have the same problem like me too ? What do you think ?
Anyway, thanks for your awesome library. You guys are my idols xD

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 4, 2020

@hdtung88 TBH, I can't follow your argumentation. If you really end up with a bigger file size, you seem to do something wrong in your build. I suggest you post this problem at stackoverflow.

@hdtung88
Copy link

hdtung88 commented Mar 4, 2020

@hdtung88 TBH, I can't follow your argumentation. If you really end up with a bigger file size, you seem to do something wrong in your build. I suggest you post this problem at stackoverflow.

There is nothing wrong with my build.
I import three.js and build it through npm. And when you want to obfuscate the final code, it really get bigger size because obfuscators convert the whole three.js library into hexadecimals.
But If you use the version three.min.js and import via <script> tag It will not be converted to hex.
I have a problem and I posted it, thought maybe it's helpful for someone. If it turned into annoying for you, then Im sorry. Please let it go and close this.
Thank you for answering and your works.

@looeee
Copy link
Collaborator

looeee commented Mar 4, 2020

I have a problem and I posted it

Can you make a post on the forum? We can discuss your problem there.

Please include a code sample that demonstrates the problem so that other people can test it, if possible.

If it turns out this is actually impossible to solve without using three.min.js then we can report our findings back here.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Apr 24, 2020

This process seems to be stalled. Is there a next step that I or others could help with?

@mrdoob
Copy link
Owner

mrdoob commented May 22, 2020

Sorry for the delay.

I think this PR is good. Just a few conflicts to fix.

Regarding the message...

THREE.WebGL: Importing from 'examples/js' is now deprecated. Please import ES6 modules from 'examples/jsm' instead. Refer to the announcement and documentation, or reach out on the forums if you need guidance with this change. Announcement: TODO. Documentation: https://threejs.org/docs/index.html#manual/en/introduction/Import-via-modules." );
/

How about something like this?

As part of the transition to ES6 Modules, the files in 'examples/js' have been deprecated in r117 (May 2020) and will be deleted in r124 (December 2020). You can find more information about developing using ES6 Modules in https://threejs.org/docs/index.html#manual/en/introduction/Import-via-modules.

@mrdoob mrdoob added this to the r117 milestone May 22, 2020
@WestLangley
Copy link
Collaborator

How about including Deprecated.js, instead?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 22, 2020

How about including Deprecated.js, instead?

Um, but this approach won't automatically log warnings when users import files from the exampels/js directory. It seems adding a console.warn to each file is the most effective way to inform users about the legacy status.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 22, 2020

@mrdoob I've solved the conflicts and added the new text.

@mrdoob mrdoob merged commit abac1e6 into mrdoob:dev May 27, 2020
@mrdoob
Copy link
Owner

mrdoob commented May 27, 2020

Thanks!

@WestLangley
Copy link
Collaborator

modularize.js copies this text to the jsm/files.

Uh oh... in fact it appears to be worse than that...

@mrdoob
Copy link
Owner

mrdoob commented May 27, 2020

I think this PR included a change to modularize.js. Maybe it got lost in the last update?

@donmccurdy
Copy link
Collaborator

/cc #19473

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 27, 2020

Maybe it got lost in the last update?

Yes, sorry. The last force-push has overwritten this change.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 28, 2020

@donmccurdy We discussed a bit earlier that it might be a good idea to explain users the reasons of this PR a bit. Especially since the removal of examples/js can be considered as a major change. Regarding #18749 (comment), it seems now is the right time to draw some attention to this topic in the forum. What do you think?

@donmccurdy
Copy link
Collaborator

Yes, agreed. I will write something up.

@marquizzo
Copy link
Contributor

marquizzo commented Jun 4, 2020

@Mugen87 I just ran into this deprecation message for the first time and was a bit confused because it says

... 'examples/js' will be deleted in December 2020.

but it doesn't give you an alternative solution. You have to copy-paste the URL and scroll to "Importable Examples" at the bottom of that page before the user can learn what to do. Wouldn't it be more user-friendly if the suggested course-of-action was part of the warning itself?

Please import via modules from 'examples/jsm'.

@donmccurdy
Copy link
Collaborator

@marquizzo I'll write up a more detailed post about this and put it in the forum (see above). Perhaps we should link there from the deprecation message, once it's up.

@marquizzo
Copy link
Contributor

Right, I understand there'll be a more formal post explaining why in the near future. But shouldn't the warning also say "please import via modules from 'examples/jsm'"? I feel that all Three.js warnings give you the solution as part of the warning itself, but this one makes you look for the solution elsewhere.

Anyway, just my suggestion 🙂.

@donmccurdy
Copy link
Collaborator

I think that's what the deprecation warning is trying to say, yes, but maybe it could be clearer.

As convenient as it is to have detailed explanations in error messages, large strings of text don't minify well, so that's a tradeoff. This example probably matters less because the messages are (a) not in the core library, and (b) all the same so Gzip may help more.

@donmccurdy
Copy link
Collaborator

Opened #19622.

nikolas added a commit to ccnmtl/astro-simulations that referenced this pull request Oct 13, 2020
This resolves a deprecation warning, introduced in three.js in May:
mrdoob/three.js#18749
nikolas added a commit to ccnmtl/astro-simulations that referenced this pull request Oct 13, 2020
This resolves a deprecation warning, introduced in three.js in May:
mrdoob/three.js#18749
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.

7 participants