Refactor to use a single sandboxed context per visit request. #236
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kratiahuja
requested changes
Oct 26, 2019
rwjblue
force-pushed
the
build-vm-context-per-visit
branch
2 times, most recently
from
f90d10a to
85f0392
Compare
Co-authored-by: Kris Selden <[email protected]>
Co-authored-by: Kris Selden <[email protected]>
Avoid reevaluating the app files and vendor files from scratch for each visit request. Co-authored-by: Kris Selden <[email protected]>
…nce`) * Collapse `createEmberApp` into `buildApp` (since we are no longer separating the concepts of `Ember.Application` vs `Ember.ApplicationInstance`) * Stash the `Ember.Application` instance (along with the `Ember.ApplicationInstance` instance) after creation, for destruction later * Ensure destruction of both `Ember.Application` and `Ember.ApplicationInstance` occur when the `visit` has completed.
This helps ensure that if the dist files themselves are invalid, we fail fast (e.g. before the first `fastboot.visit()` invocation).
rwjblue
force-pushed
the
build-vm-context-per-visit
branch
from
3cc609a to
5a1b968
Compare
kratiahuja
reviewed
Oct 30, 2019
kratiahuja
reviewed
Oct 30, 2019
kratiahuja
approved these changes
Oct 30, 2019
Contributor
|
@rwjblue Thanks for making these changes, these look good to me except the two questions above. |
kratiahuja
reviewed
Oct 30, 2019
added 2 commits
October 30, 2019 19:23
…t requests. This shows that mutating the prototype of an object used in multiple requests, leaks that mutation.
rwjblue
force-pushed
the
build-vm-context-per-visit
branch
from
5a1b968 to
0893cec
Compare
rwjblue
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
vm.createContextper visitadd this back if needed, but the complexity it adds seems quite high
and very very few (if any) folks use it).
Co-authored-by: @krisselden
TODO:
src/ember-app.jsto do a "simple"app.boot()to ensure that parsing failures themselves will fail the node processEmber.onerroris unique per-visit (Failing test: Ember.onerror setup via instance initailizer leaks across visits #242)Follow-up TODO:
sandboxGlobalstobuildSandboxGlobals(RefactorsandboxGlobalsoptions intobuildSandboxGlobals#239)destroyAppInstanceInMs(RefactordestroyAppInstanceInMssystem #240)Closes #241
Closes #242