Closed jmbejar closed 6 years ago
What browser are you testing in?
Hmm, seems like a mistake to me. Shouldn’t it be self.Promise.resolve().then(callback)
? Mind testing that to confirm?
@rwjblue I tested with self.Promise.resolve().then(callback)
. There is no exception, but the component does not render.
With your change, the callback
is immediately invoked and the code to render the component in the callback does not work.
I think that the idea with the code before 319ffc9 was to invoke this callback once app['_rendering']
changes to false.
I tracked down the code and found this https://github.com/glimmerjs/glimmer.js/blob/b48212df42f0ef630f2f184b616635540a40e1bc/packages/%40glimmer/application/src/application.ts#L181.
I see that invoking the callback with setTimeout should just work, but maybe using a promise here is better for some reason?
Maybe @mixonic could have a better idea about the intention in 319ffc9 and provide some clues about the issue.
Note: testing with Chrome 60.0.3112.113 in MacOS
That change absolutely is in error. https://github.com/glimmerjs/glimmer-web-component/pull/21#discussion_r138737677
But yes even that seems wrong, as it would hang the browser.
I'll open a new PR.
@jmbejar I opened https://github.com/glimmerjs/glimmer-web-component/pull/25 with the minimal change.
Glimmer's rendering is synchronous. The aim here was that we could use promise resolution to ensure the check for _rendering
happens after the current stack (and rendering) completes. This would only infinite loop if rendering was never complete, and I'm not sure what would cause a blank screen.
@mixonic I tested again your changes and this is what I'm seeing (the infinite loop commented by #21 (comment)):
Please note the stack trace, it hangs the browser tab because of an infinite loop. It looks like self.Promise.resolve()
returns an already resolved promise and this is invoking the then
callback immediately, hence it has no chance to change the value of _rendering
.
@mixonic This version seems to work OK
function whenRendered(app, callback) {
if (app['_rendering']) {
requestAnimationFrame(() => {
whenRendered(app, callback);
});
} else {
callback();
}
}
This is based on what I see here: https://github.com/glimmerjs/glimmer.js/blob/b48212df42f0ef630f2f184b616635540a40e1bc/packages/%40glimmer/application/src/application.ts#L181
I'm getting an error testing a Glimmer component (Glimmer 0.8) as a web component in a custom webpage (after building the GlimmerJS and including the
app.js
in a separate page):The component itself is rendered, but the error shows up after all.
Digging into the issue, I've confirmed that the code before https://github.com/glimmerjs/glimmer-web-component/commit/319ffc9827d78e65734b2d955421d0583b17c3d5 worked.
Extra information
Here is the package.json of the Glimmer.js project: