Support async System.import (should count as dependency) #129

mpfau · 2017-02-27T22:55:34Z

I'm currently switching over to the current release... :-)

Normal imports are resolved perfectly when a file is changed. However, everything imported via a System.import fails with the following error:

Error: parentKey.indexOf is not a function
  Resolving http://mpfau:9000/beta/client/build/app-import.js to 1
    at getParentMetadata (http://mpfau:9000/beta/client/node_modules/systemjs/dist/system.src.js:1587:42)
    at SystemJSLoader$1.normalize (http://mpfau:9000/beta/client/node_modules/systemjs/dist/system.src.js:1604:24)
    at http://mpfau:9000/beta/client/node_modules/systemjs/dist/system.src.js:321:27
From previous event:
    at eval (http://mpfau:9000/beta/client/node_modules/systemjs-hmr/dist/systemjs-hmr.js:12879:10)
From previous event:
    at eval (http://mpfau:9000/beta/client/node_modules/systemjs-hmr/dist/systemjs-hmr.js:12874:8)

Steps to reproduce

npm install [email protected] [email protected]
Setup app.js, app-import.js and test.html as defined below
let chokidar-socket-emitter listen to the current dir and port 9082
open test.html in a browser
make a change to app-import.js and the error described above is triggered

app.js

SystemJS.import("app-import.js")

app-import.js

console.log("Test for alexis")

test.html

<html>
<head>
    <script src="../node_modules/systemjs/dist/system.src.js"></script>
    <script>
		System.config({
			"transpiler": false,
			"map": {
				"systemjs-hot-reloader": "../node_modules/systemjs-hot-reloader/dist/index.js",
				"systemjs-hmr": "../node_modules/systemjs-hmr/dist/systemjs-hmr.js",
			}
		})

		System.import('systemjs-hot-reloader').then(function (connect) {
			connect({host: location.protocol + '//' + location.hostname + ':9082'})

			return System.import('app.js')
		})
    </script>
</head>
</html>

The text was updated successfully, but these errors were encountered:

alexisvincent · 2017-02-27T23:01:23Z

Thanks, will test tomorrow morning. This is an interesting case I haven't actually thought about. Assuming the bug you're seeing is fixed, I'm not sure how I will be able to determine that app.js has a dependency on app-import.js. Maybe @guybedford can make this show up in the trace api somehow?

mpfau · 2017-02-28T08:11:19Z

I debugged a bit and noticed that the function Loader.prototype.resolve = function (key, parent) (line 317 of system.src.js) is invoked by system-hmr with the array index of the entries array as parentKey if the deps of the module are empty.

I think that it would be nice if SystemJs would add dependencies for System.import statements, too.
As we use System.import statements only in a few places, a valid workaround would be to add the dependencies through an indirection (asyncImport) and setup the dependencies:

asyncImport(module.id, "app-import.js")

function asyncImport(importer, moduleName) {
	System.loads[System.resolveSync(importer)].deps.push(moduleName)
	System.loads[System.resolveSync(importer)].depMap[moduleName] = System.resolveSync(moduleName)
	return System.import(moduleName)
}

alexisvincent · 2017-02-28T08:49:31Z

@mpfau, that could work, but I have a feeling deps and depMap (we only use depMap) are reset every time. Might be wrong. Note, System.import optionally takes a parent address, which could be augmented to support adding values to depMap. Something like.

System.import = function(moduleName, parentName) {
    if (parentName) {
       // add to depMap 
    }
    return System.proto.import.apply(System, [moduleName, parentName])
}

Then you could import via

// see https://github.com/systemjs/systemjs/blob/master/docs/system-api.md#systemjsimportmodulename--normalizedparentname---promisemodule
System.import('./app-import.js', __moduleName)

perhaps @guybedford could rewrite System.import('moduleName') statements to System.import('moduleName', __moduleName) at compile/transpile time.

alexisvincent · 2017-02-28T08:50:05Z

Either way, having this in the trace API would be very useful.

adelespinasse · 2017-02-28T17:31:48Z

I'm getting a very similar error, but with normal ES6-style imports. In fact I think it's a basically identical error, except that the compiled version of system.js is being used instead of the debuggable source (I don't know how to change this):

Error: r.indexOf is not a function
  Resolving http://localhost:5000/src/util/userutil.js to 1
    at G (http://localhost:5000/jspm_packages/system.js:4:11953)
    at Ve.H [as normalize] (http://localhost:5000/jspm_packages/system.js:4:12147)
    at http://localhost:5000/jspm_packages/system.js:5:5036
From previous event:
    at eval (http://localhost:5000/jspm_packages/npm/[email protected]/dist/systemjs-hmr.js:11188:12)
From previous event:
    at eval (http://localhost:5000/jspm_packages/npm/[email protected]/dist/systemjs-hmr.js:11183:10)

I assumed I was doing something wrong, but seeing this bug report made me not so sure.

The first line of the stack trace (userutil.js line 1) seems unrelated to which source file was modified. The line referenced is import md5 from 'blueimp-md5'; (importing a 3rd-party npm package). I assume it just happens to be the first node in the dependency graph or something. userutil.js is always imported via an ES6-style import when other files use it.

The above is what I get the first time a source file is modified after the app is loaded. After that any additional changes result in this error:

TypeError: Cannot read property '__reload' of undefined
    at eval (http://localhost:5000/jspm_packages/npm/[email protected]/dist/systemjs-hmr.js:11167:104)
From previous event:
    at eval (http://localhost:5000/jspm_packages/npm/[email protected]/dist/systemjs-hmr.js:11154:25)

alexisvincent · 2017-02-28T17:36:21Z

Fix coming in the next 30 min or so.

alexisvincent · 2017-02-28T17:41:00Z

Just recovering from a typo. Ran rm -rf ~ instead of rm -rf ./~. Destroyed some of my home dir...

alexisvincent · 2017-02-28T18:06:08Z

Fixed in systemjs-hmr 2.0.8.

alexisvincent · 2017-02-28T18:06:45Z

Refurbishing this issue for async import support

mpfau · 2017-03-01T11:11:33Z

@alexisvincent thanks for fixing! I just tested successfully against system-hmr 2.0.8.

It looks like deps and depMap are reset every time. However, that is not a problem as the file is re-imported on change (and the dependencies are re-written to deps/depMap).

It would be nice, if @guybedford could adapt System.import according to your ideas when System.trace == true.

alexisvincent · 2017-03-01T15:39:10Z

Something else we could do is save these import mappings in a side table. However I think the best place for this would belong in the ES-Module-Loader trace implementation. @guybedford, would appreciate your input here.

guybedford · 2017-03-01T16:14:42Z

I've been experimenting with a dynamic deps trace in ModuleLoader/es-module-loader@4f23167.

What we can do is catch all calls to the dynamic import (not System.import, but the contextual import when compiled with System.register) and know that it was made by the parent module.

I'm also not sure what use this information is to the hot reloader as well - there's no way to refresh the references once they have been returned from the import promise?

Perhaps just a direct tracing of top-level imports is all that is needed.

alexisvincent · 2017-03-01T20:54:13Z

@guybedford Awesome! Is there a reason that you would only support this for import and not System.import when a parentKey is provided?

This would work for the hot-reloader as follows. In the example that @mpfau at the top of this issue, by listing app-import.js in the depMap entry of app.js, upon a change event for app-import.js, app.js would be detected as an "importer" of app-import.js, thus both would be deleted from the registry. Then app.js would be detected as the only dependency root and would be imported, which would call System.import('app-import.js'), which no longer exists in the registry and so would be reimported. Exactly as we would expect.

Not sure what you mean by

Perhaps just a direct tracing of top-level imports is all that is needed.

guybedford · 2017-03-09T12:23:57Z

Ok, then the implementation I've got there should be a good start to play around with. It's not possible to tell when System.import is run within which module, as there is no global context available to get its calling site. So this is only something we can support for the import method.

System.import with a parent key can be used for arbitrary relative normalization, without implying that it is within the given module.

alexisvincent · 2017-03-09T13:04:01Z

Right, of course. Awesome, would this be able to make its way into a version of SystemJS? Perhaps via a flag, for general testing?

guybedford · 2017-03-09T13:19:30Z

@alexisvincent this will be shipping in the next SystemJS release, which should be soon.

alexisvincent closed this as completed Feb 28, 2017

alexisvincent reopened this Feb 28, 2017

alexisvincent changed the title ~~async imports / System.import not resolving on reload~~ Support async System.import (should count as dependency) Feb 28, 2017

alexisvincent mentioned this issue Feb 28, 2017

Does not reload components via (angular2) async routes #49

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support async System.import (should count as dependency) #129

Support async System.import (should count as dependency) #129

mpfau commented Feb 27, 2017

alexisvincent commented Feb 27, 2017

mpfau commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

adelespinasse commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

mpfau commented Mar 1, 2017

alexisvincent commented Mar 1, 2017

guybedford commented Mar 1, 2017

alexisvincent commented Mar 1, 2017

guybedford commented Mar 9, 2017

alexisvincent commented Mar 9, 2017

guybedford commented Mar 9, 2017

Support async System.import (should count as dependency) #129

Support async System.import (should count as dependency) #129

Comments

mpfau commented Feb 27, 2017

Steps to reproduce

app.js

app-import.js

test.html

alexisvincent commented Feb 27, 2017

mpfau commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

adelespinasse commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

alexisvincent commented Feb 28, 2017

mpfau commented Mar 1, 2017

alexisvincent commented Mar 1, 2017

guybedford commented Mar 1, 2017

alexisvincent commented Mar 1, 2017

guybedford commented Mar 9, 2017

alexisvincent commented Mar 9, 2017

guybedford commented Mar 9, 2017