refactor(loader-webpack): refactored aysnc await code to use promises#34
refactor(loader-webpack): refactored aysnc await code to use promises#34suneelv wants to merge 2 commits into
Conversation
|
|
jods4
left a comment
jods4
left a comment
There was a problem hiding this comment.
I made a few comments.
Did you double check that all await expressions were guaranteed to be Promise?
I'm asking because it is legal to await something that may not be a Promise, in which case the code behaves like await 42 ==> await Promise.resolve(42)
| if (!entry.templateIsLoaded) { | ||
| await this.templateLoader.loadTemplate(this, entry); | ||
| return this.templateLoader.loadTemplate(this, entry).then(() => { | ||
| return entry; |
| loadTemplate(loader: Loader, entry: TemplateRegistryEntry) { | ||
| return loader.loadText(entry.address).then((text) => { | ||
| entry.template = DOM.createTemplateFromMarkup(text); | ||
| }) |
| await this.hmrContext.handleViewChange(moduleId); | ||
| module.hot.accept(moduleId, () => { | ||
| return this.hmrContext.handleViewChange(moduleId).then((resource) => { | ||
| return resource; |
There was a problem hiding this comment.
Why return resource?
.then(x => x) is a no-op, isn't it?
| } | ||
| return await plugin.fetch(moduleId); | ||
| return plugin.fetch(moduleId).then((resource) => { | ||
| return resource; |
There was a problem hiding this comment.
Same question as above: isn't .then(res => res) a no-op?
| this.moduleRegistry[moduleId] = ensureOriginOnExports(moduleExports, moduleId); | ||
| this.modulesBeingLoaded.delete(moduleId); | ||
| return moduleExports; | ||
| // const moduleExports = await beingLoaded; |
There was a problem hiding this comment.
Don't leave comments behind, git history is there for that.
| this.modulesBeingLoaded.delete(moduleId); | ||
| return moduleExports; | ||
| // const moduleExports = await beingLoaded; | ||
| return beingLoaded.then((moduleExports) => { |
There was a problem hiding this comment.
Just saying: when you've got a single argument, braces are optional (I find it more legible without).
| this.moduleRegistry[moduleId] = ensureOriginOnExports(moduleExports, moduleId); | ||
| this.modulesBeingLoaded.delete(moduleId); | ||
| return moduleExports; | ||
| }) |
| } | ||
| return result; | ||
| loadText(url: string) { | ||
| return this.loadModule(url,false).then((result) => { |
There was a problem hiding this comment.
Space in url,false was lost.
| return result.toString(); | ||
| } | ||
| return result; | ||
| }) |
|
@jods4 Thanks for the comments. I am new to this. I will update the pull request with changes. |
|
@jods4 Yes I am always returning promises from all the old async methods. I am not really familiar with what loader-webpack is doing. So it would be better if you can just skim through it once again. |
|
Not requiring the async downlevel helpers from TS will make the code smaller... but sadly the stack overflow is still not fixed :( |
|
@niieani Is this good to merge then? |
|
@EisenbergEffect It would be good to test against the skeleton (I haven't) but otherwise looks good. Perhaps we can release tagged as beta or RC first, and if no issues show, re-release as final. |
|
@jods4 Have we tested this? What should we do with this PR? |
|
@EisenbergEffect The runtime loader is really @niieani's work. I have reviewed the PR and it looks good, I haven't tested it, though. |
|
Yes, looks good as @jods4 said, but I haven't tested either. |
|
Has anyone tested this out yet? |
5 participants
In response to aurelia/skeleton-navigation#821
FYI This didn't prevent the out of stack space error in ie11