-
Notifications
You must be signed in to change notification settings - Fork 75
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
Separate Application responsibilities #34
Conversation
e6875c5
to
8c2b3f3
Compare
stdio: 'inherit', | ||
preferLocal: true | ||
}); | ||
} catch (e) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the error at least be logged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
execa throws an exception if the return value is non-zero, so you end up with a useless stack trace every time the tests fail. Since we pipe stderr to the parent stderr you will see the error messages anyway. You're right though that we should probably check to verify that the error isn't caused by a bad path or something and is really a non-zero exit code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context. Even if nothing is handled, a comment would probably help in regards to why the error is ignored 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomdale - Can you confirm that failing node tests properly fail the build now? My concern is that swallowing this error, will "eat" the non-zero exit code and the tests would seen as "passing" (due to process exiting with 0 exit code) when they actually failed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to not repeat what is done in the VM when it comes to swallowing the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, indeed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knownasilya The release version of execa hides all information from you in the case of a non-zero exit code in the sync case, which is really unfortunate. It looks like a PR was accepted 6 days ago to address this but hasn't been released yet.
@rwjblue In my tests, running yarn test
with a failing Node test produces an exit code of 1
, so I think we're good. I would expect Testem to treat TAP output that contained not ok
as a failure, which it seems to be doing.
8c2b3f3
to
9e0c888
Compare
env.commit(); | ||
this._didRender(); | ||
}); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need to guard all of this with if (result) { ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The guard is in SyncRenderer
; the Application
no longer knows about the render result.
@@ -231,38 +286,12 @@ export default class Application implements Owner { | |||
* at the end of the browser's event loop. | |||
*/ | |||
protected _rerender() { | |||
let { env, _result: result } = this; | |||
let { env } = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} while (!result.done); | ||
|
||
// Finally, commit the transaction and flush component lifecycle hooks. | ||
this.renderer.rerender(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-render is synchronous but render is async? Seems like these should be balanced or at least check for the balancing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, rerender cannot be async at the moment. The interface for rerender()
already supports a Promise return type, but there aren't any implementations that can test this path yet.
import { Cursor, NewElementBuilder, Environment } from "@glimmer/runtime"; | ||
import { Builder } from "../application"; | ||
|
||
export interface DOMBuilderOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a Cursor
and the type is exposed from @glimmer/runtime
as such. If you want a type alias for the same thing just extend
it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chadhietala Deny, Cursor
does not allow nextSibling
to be undefined
. I don't think we want to make users have to explicitly set nextSibling
to null
every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is an Option
. In the constructor just do { element, nextSibling = null }
This commit breaks the Application's responsibilities into three composable objects: 1. A Loader, which may load and/or compile an application's templates. 2. A Builder, which determines how template output is built (e.g. building DOM). 3. A Renderer, which schedules rendering work.
8976fe3
to
773e7b7
Compare
Woot! |
As @chadhietala noted in #31, as the Glimmer VM has continued to expand its platform of capabilities, supporting the matrix of configuration options is becoming increasingly difficult.
Adding to the difficulty is the fact that some of the "modes" carry much more code than others. We need an API that makes configuration as simple as possible, while ensuring that the final payload only includes code for modes that are actually used.
This commit breaks the majority of the Application's responsibilities into three composable objects:
Examples
Today
Today's Glimmer.js app has the following characteristics:
In the API proposed here, the bootstrapping code would look like this:
This is very similar to the code generated by the blueprint today. The only change is that, instead of passing the resolver and element to the Application constructor directly, we wrap them in a RuntimeLoader and DOMBuilder, respectively. We then pass the RuntimeLoader, DOMBuilder and SyncRenderer to the Application constructor.
So far, we've added a little bit of boilerplate for the same semantics. Not much of an improvement! Let's see where this gets more interesting.
Bytecode
Imagine we now want to move the final bytecode compilation step out of the browser and into our build process. To do this, we will compile our templates into a single binary
.gbx
file. Additionally, we want to incrementally perform the initial render, so lower-end devices stay responsive even if our DOM is large.Now we can reconfigure our application like this:
With very similar code to the example above, we've now radically changed how our application behaves. It will fetch the binary bytecode and pass it to the BytecodeLoader, which is much smaller and faster than the RuntimeLoader. Because we pass in the AsyncRenderer, the initial render will be batched via
requestIdleCallback
so the browser stays responsive, no matter how many frames it takes to render the whole page.Additional Examples
For additional examples of how this API may be used to fine-tune the performance of a Glimmer.js application, see this gist.
Additional Changes
This PR is intentionally scoped as small as possible, with the hope that it can be merged early to avoid a long-lived branch. There are several shortcomings in the implementation here that I intend to address before we release:
boot()
etc.).