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

A simple implementation of Angular 2 templateUrl style component hot-… #73

Closed
wants to merge 1 commit into from

Conversation

herdemir
Copy link

A simple solution for templateUrl style Angular 2 hot-reloading issue: #62

@guybedford
Copy link
Collaborator

I wouldn't advise assuming default behaviour per extension.

@capaj
Copy link
Collaborator

capaj commented May 25, 2016

@guybedford I would not want to hardcode behaviour based on an extension as well. Well have to find a different way to solve this.

@guybedford
Copy link
Collaborator

@capaj just let me know if you want to discuss the plugin reloading further at all. I assume all these issues are to do with the ! plugin syntax form? Carefully handling the normalization of that should be all that is needed in theory.

@capaj
Copy link
Collaborator

capaj commented May 25, 2016

@guybedford I think supporting only plugins via meta config as we've been doing so far is sufficient. We should make it clearer in the readme and include some working examples where we can just link everyone to that. Angular 2 example with hot reloaded templates will be my goal.

@guybedford
Copy link
Collaborator

@capaj yeah a lot of users use plugin syntax, so this should either be highlighted at the top of the docs as something not supported or it should definitely be supported. Handling plugins uses the same System.normalize functionality, with the URL to be taken as load.name.split('!')[0].

@capaj
Copy link
Collaborator

capaj commented May 25, 2016

oh, I did not realize plugin syntax would be so easy to handle. I will try to add support for it then.

@guybedford
Copy link
Collaborator

I guess the main thing is to do the one to many reverse look up, where a change to file at address should reload all of address, addres!* modules.

@capaj
Copy link
Collaborator

capaj commented May 25, 2016

@guybedford thanks for pointing me to it. Sounds like a good idea, even though this could lead to situation where we reload more than is needed.
Like when you'd have widget.js, widget.css and widget.html, then changing widget.js would reload all three files. But better that, than now.

@guybedford
Copy link
Collaborator

guybedford commented May 25, 2016

@capaj if you have widget.js, widget.css and widget.html then say the CSS file was loaded with plugin meta, while the HTML file was loaded with plugin syntax. Then the names in the registry are of the form //.../path/to/widget.js, //.../path/to/widget.css, //.../path/to/widget.html!//.../path/to/html-plugin.js. Changing widget.js or widget.css can only update the right file. Only changing widget.html could possible reload all plugin variations of widget.html as well as widget.html without the plugin syntax.

@capaj
Copy link
Collaborator

capaj commented May 26, 2016

@guybedford I tried it on jspm-ng2 repo and found out that when I call:

System.normalize('src/hello.html!').then((a)=>{console.log(a)})
http://localhost:9089/src/hello.html!http://localhost:9089/html.js

But my template file is stored under a key

"http://localhost:9089/src/hello.html!http://localhost:9089/jspm_packages/github/systemjs/plugin-text@0.0.8.js"

Meaning when I want to delete it, I need to call:

System.delete("http://localhost:9089/src/hello.html!http://localhost:9089/jspm_packages/github/systemjs/plugin-text@0.0.8.js")

Why is System.normalize working as it works for plugins? Why doesn't it normalize fully?

The way it works now, it seems to me that the only way how to find plugin loaded modules is to iterate trough System.loads and try to match to my string. This could get slow on bigger projects.

@guybedford
Copy link
Collaborator

@capaj yes plugin syntax normalizes uniquely based on the unique module name of the plugin. Yes you would need to iterate all loads to match all possible plugins for a given invalidation. It would be worth checking the performance but I don't expect it would be that bad?

@capaj
Copy link
Collaborator

capaj commented May 26, 2016

@guybedford ok, you're right. It will be plenty fast. I will give it a shot.

@capaj
Copy link
Collaborator

capaj commented May 26, 2016

@guybedford
I have modified

getModuleRecord

to match the record needed. That works and finds the right module to delete. When it is deleted, it will be reimported and reloaded properly.
Although there is one issue-when I load the jspm-ng2 app for the first time and look at the module by invoking System.loads I get this:
image

the bad thing is that is lacks importers. I am importing the template from the app.js file, so it should have an importers property, yet it does not have one. When I add blank line in app.js and it hot-reloads, importer suddenly appears in the module:

image

Any idea why that might be ?

@capaj capaj mentioned this pull request May 26, 2016
@guybedford
Copy link
Collaborator

@capaj I don't think we set importers on trace records do we? At least it's certainly not set in https://github.com/ModuleLoader/es6-module-loader/blob/master/src/loader.js#L589.

@capaj
Copy link
Collaborator

capaj commented May 26, 2016

@guybedford Oh I just realized that I clicked the wrong property on System.loads array. Silly.

@guybedford
Copy link
Collaborator

Those importers shouldn't be set anywhere in SystemJS or the module loader!

@jonricaurte
Copy link

I think if you are using jspm/systemjs you should not be using templateUrl and instead be importing the html.

@capaj
Copy link
Collaborator

capaj commented Oct 30, 2016

conflicted and should be reopened in https://github.com/alexisvincent/systemjs-hmr if still needed.

@capaj capaj closed this Oct 30, 2016
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.

None yet

4 participants