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

core: Loaders to es6 classes #19985

Merged
merged 4 commits into from
Feb 6, 2021

Conversation

DefinitelyMaybe
Copy link
Contributor

@DefinitelyMaybe DefinitelyMaybe commented Aug 1, 2020

supported by #19982 i.e. managed to build without errors after implementing those changes.

Did run npm test but we will see what comes back from the automated tests. I'm feeling slightly weary of the current testing, future me problem.

note:
Many loaders have the following pattern:

class NewLoader extends Loader {
  // ...
  load( url, onLoad, onProgress, onError ) {

    const scope = this;
    // ...
    const loader = new FileLoader( this.manager );
    loader.setPath( this.path );
    loader.setRequestHeader( this.requestHeader );
    loader.load( url, function ( text ) {
    // ...
    }, onProgress, onError);
  }
}

It doesn't look very good to me however instead of trying to tackle that within the same PR I'll follow up on this one in a future PR.

@DefinitelyMaybe DefinitelyMaybe changed the title core: Loaders to es6 classes Draft: core: Loaders to es6 classes Aug 1, 2020
@DefinitelyMaybe DefinitelyMaybe mentioned this pull request Aug 2, 2020
43 tasks
@mrdoob mrdoob added this to the r120 milestone Aug 2, 2020
src/loaders/Loader.js Outdated Show resolved Hide resolved
@DefinitelyMaybe
Copy link
Contributor Author

as #19982 has been merged, I can either add example loaders to this PR or see if I can get the tests to re-run. might need to do a dummy commit but we'll see.

@DefinitelyMaybe DefinitelyMaybe marked this pull request as draft August 3, 2020 04:35
@DefinitelyMaybe DefinitelyMaybe force-pushed the loaders--move-to-es6-classes branch from 734341e to e7f3d7d Compare August 15, 2020 03:12
@DefinitelyMaybe DefinitelyMaybe marked this pull request as ready for review August 15, 2020 03:26
@DefinitelyMaybe DefinitelyMaybe changed the title Draft: core: Loaders to es6 classes core: Loaders to es6 classes Aug 15, 2020
@DefinitelyMaybe
Copy link
Contributor Author

removed changes to loaders which were used by any example script.

@DefinitelyMaybe DefinitelyMaybe requested a review from mrdoob August 17, 2020 08:29
@mrdoob mrdoob modified the milestones: r120, r121 Aug 23, 2020
@mrdoob mrdoob modified the milestones: r121, r122 Sep 29, 2020
@mrdoob mrdoob modified the milestones: r122, r123 Oct 28, 2020
@DefinitelyMaybe
Copy link
Contributor Author

conflicts resolved

@mrdoob mrdoob modified the milestones: r123, r124 Nov 25, 2020
@mrdoob mrdoob modified the milestones: r124, r125 Dec 24, 2020
@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021
@mrdoob mrdoob merged commit 73d1dde into mrdoob:dev Feb 6, 2021
@mrdoob
Copy link
Owner

mrdoob commented Feb 6, 2021

Thanks!

@DefinitelyMaybe DefinitelyMaybe deleted the loaders--move-to-es6-classes branch February 7, 2021 09:02
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