Extend TraceurLoader to BrowserTraceurLoader and NodeTraceurLoader. #1802

johnjbarton · 2015-03-10T00:02:17Z

Move src/runtime/System.js to src/browser/System.js.
Adjust test paths for root base url.
Test new System.constructor() to create a secondary loader.

arv · 2015-03-10T14:06:13Z

src/runtime/TraceurLoader.js

@@ -287,4 +289,16 @@ export class TraceurLoader extends Loader {
      factoryFunction);
  }

+  static createLoader() {


Export this as a function instead?

export function createLoader() { ... }

I like TraceurLoader.createLoader() as API.

arv · 2015-03-10T14:07:02Z

Travis failed.

johnjbarton · 2015-03-10T15:32:40Z

The Travis problem is likely issue #1804, commander version.

arv · 2015-03-11T01:40:55Z

src/node/NodeLoaderCompiler.js

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+import {LoaderCompiler} from '../runtime/LoaderCompiler.js';


Up until now src/node/** does not contain ES6. Maybe move this file to src/runtime/?

Yes, that should work. The other distinction of src/node/ is that it is shipped in the npm package. But this file will be compiled in to bin/traceur.js.

Done in next commit

arv · 2015-03-11T01:41:11Z

LGTM

guybedford · 2015-03-11T09:45:57Z

I don't mean to intervene, but was just curious what use case this API provides over standard class instantiation? While I know it is a far off dream, ensuring minimal APIs added to the loader above those in the spec makes it more likely loaders will be interchangeable.

johnjbarton · 2015-03-11T14:28:29Z

The use case is to create a new Loader. I'm open to alternatives. Our loader uses a platform dependent fileLoader and base url, so these have to come from somewhere.

guybedford · 2015-03-11T14:42:41Z

Perhaps this could be done through BrowserLoader and NodeLoader extending TraceurLoader, whose instances then get assigned to the System namespace in both environments?

guybedford · 2015-03-11T14:43:13Z

Not into this code well enough to comment too deeply, but just wanted to mention my concerns over the API drift.

johnjbarton · 2015-03-11T14:54:52Z

This function detects whether it is running in browser or node and creates a loader for the appropriate environment. Its purpose is provide a platform-independent mechanism to create a Loader. That way code that needs a Loader does not need to add detection code every time.

The use case is obtaining a Loader for testing. The System reference is not used, though it would be great is we had something on the standard for creating a secondary Loader.

guybedford · 2015-03-11T15:01:38Z

@johnjbarton the assumtion I've always made is that the local System is an instance of the environment loader. This reference effectively then provides that in a way that will be provided by the spec.

For creating a secondary loader we do have new System.constructor for that.

johnjbarton · 2015-03-11T15:06:02Z

For creating a secondary loader we do have new System.constructor for that.

Where is that discussed?

guybedford · 2015-03-11T15:08:49Z

It's just how classes work?

guybedford · 2015-03-11T15:10:34Z

The discussion was from whatwg/loader#7.

johnjbarton · 2015-03-11T15:29:37Z

But here are the (lack of) arguments to the constructor specified? I did not know there was even any spec for a constructor.

guybedford · 2015-03-11T15:38:44Z

The fetch implementation is managed as a hook not a constructor argument - loader.hook('fetch', implementation) is the current spec for this, which would then run within the constructor itself for the browser loader I think.

sebmck · 2015-03-12T00:32:31Z

src/node/traceur.js

@@ -22,7 +22,7 @@ if (!data)
  throw new Error('Failed to import ' + filename);

 module._compile(data, filename);
-
+console.log('traceur ', traceur.runtime);


Yes, I had to commit to move the code to another machine. Still in progress.

johnjbarton · 2015-03-12T00:49:47Z

PTAL

@guybedford Thanks for the input, better for sure. I'll try to clean up the code now, to see if I can move the fileLoader out of the base class.

arv · 2015-03-12T15:50:04Z

src/runtime/System.js

-}
-
-var traceurLoader = new TraceurLoader(fileLoader, url);
+let traceurLoader = new BrowserTraceurLoader();


Is this correct? Now System is always the browser loader.

src/node/System.js is used in node code. Browser code gets this file by module loading. Maybe we need a src/browser/ to make this clearer?

+1 to src/browser

…across platforms. The feature tests use a sepearate loader so es6-ifying them will be easier with a better loader creator.

Adjust test paths for root base url

johnjbarton · 2015-03-29T17:35:36Z

PTAL

The browser tests don't work, but they've not worked for a bit and I'm working on that now.

arv · 2015-03-29T18:10:52Z

test/unit/runtime/Loader.js

-        'import * as c from "./resources/test_c.js";\n' +
+        'import * as a from "./test/unit/runtime/resources/test_a.js";\n' +
+        'import * as b from "./test/unit/runtime/resources/test_b.js";\n' +
+        'import * as c from "./rtest/unit/runtime/esources/test_c.js";\n' +


typo: rtest

Done, I just remove the imports. This test was only to verify that we took the reject() path.

Add a prefix string on anonymous file names to make debugging them easier

arv · 2015-03-30T18:15:00Z

LGTM

arv added the in progress label Mar 10, 2015

arv reviewed Mar 10, 2015
View reviewed changes

johnjbarton force-pushed the create-loader branch from 5b42bbe to 9958507 Compare March 11, 2015 00:58

arv reviewed Mar 11, 2015
View reviewed changes

johnjbarton mentioned this pull request Mar 11, 2015

Specify the (lack of) constructor arguments. whatwg/loader#33

Closed

sebmck reviewed Mar 12, 2015
View reviewed changes

johnjbarton force-pushed the create-loader branch from 8e1a535 to 4b16e4a Compare March 12, 2015 00:39

johnjbarton changed the title ~~Add TraceurLoader.createLoader() to simplify the creation of loaders acr...~~ Extend TraceurLoader to BrowserTraceurLoader and NodeTraceurLoader. Mar 12, 2015

arv reviewed Mar 12, 2015
View reviewed changes

johnjbarton added 4 commits March 25, 2015 10:35

Add TraceurLoader.createLoader() to simplify the creation of loaders …

a6ded4f

…across platforms. The feature tests use a sepearate loader so es6-ifying them will be easier with a better loader creator.

respond to review

8826995

Extend TraceurLoader for Browser and Node

ece7ed4

respond to review

504f768

Move src/runtime/System.js to src/browser/System.js

2f95b43

johnjbarton force-pushed the create-loader branch from 87f8d63 to 85080e1 Compare March 25, 2015 17:52

BrowserTraceurLoader and NodeTracerLoader extend TraceurLoader.

7d7bd75

Adjust test paths for root base url

johnjbarton force-pushed the create-loader branch from 85080e1 to 7d7bd75 Compare March 26, 2015 23:17

self-review

8af413a

arv reviewed Mar 29, 2015
View reviewed changes

Remove extraneous part of LoaderModuleFail test.

7716873

Add a prefix string on anonymous file names to make debugging them easier

johnjbarton closed this in 9df89c6 Mar 31, 2015

arv removed the in progress label Mar 31, 2015

johnjbarton deleted the create-loader branch May 4, 2015 23:33

johnjbarton restored the create-loader branch May 12, 2015 23:32

johnjbarton deleted the create-loader branch January 29, 2016 02:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend TraceurLoader to BrowserTraceurLoader and NodeTraceurLoader. #1802

Extend TraceurLoader to BrowserTraceurLoader and NodeTraceurLoader. #1802

johnjbarton commented Mar 10, 2015

arv Mar 10, 2015

johnjbarton Mar 11, 2015

arv commented Mar 10, 2015

johnjbarton commented Mar 10, 2015

arv Mar 11, 2015

johnjbarton Mar 12, 2015

arv commented Mar 11, 2015

guybedford commented Mar 11, 2015

johnjbarton commented Mar 11, 2015

guybedford commented Mar 11, 2015

guybedford commented Mar 11, 2015

johnjbarton commented Mar 11, 2015

guybedford commented Mar 11, 2015

johnjbarton commented Mar 11, 2015

guybedford commented Mar 11, 2015

guybedford commented Mar 11, 2015

johnjbarton commented Mar 11, 2015

guybedford commented Mar 11, 2015

sebmck Mar 12, 2015

johnjbarton Mar 12, 2015

johnjbarton commented Mar 12, 2015

arv Mar 12, 2015

johnjbarton Mar 12, 2015

arv Mar 12, 2015

johnjbarton commented Mar 29, 2015

arv Mar 29, 2015

johnjbarton Mar 30, 2015

arv commented Mar 30, 2015

Extend TraceurLoader to BrowserTraceurLoader and NodeTraceurLoader. #1802

Extend TraceurLoader to BrowserTraceurLoader and NodeTraceurLoader. #1802

Conversation

johnjbarton commented Mar 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arv commented Mar 10, 2015

johnjbarton commented Mar 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arv commented Mar 11, 2015

guybedford commented Mar 11, 2015

johnjbarton commented Mar 11, 2015

guybedford commented Mar 11, 2015

guybedford commented Mar 11, 2015

johnjbarton commented Mar 11, 2015

guybedford commented Mar 11, 2015

johnjbarton commented Mar 11, 2015

guybedford commented Mar 11, 2015

guybedford commented Mar 11, 2015

johnjbarton commented Mar 11, 2015

guybedford commented Mar 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnjbarton commented Mar 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnjbarton commented Mar 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arv commented Mar 30, 2015