Expose option to allow a new sandbox per visit #252
Conversation
rwjblue
force-pushed
the
make-single-sandbox-per-request-optional
branch
from
a565cbe
to
d5ebdf0
Compare
This PR does a few things: Firstly, it allows the end consumer to decide if they want a sandbox per visit or to share a single sandbox over all visits. Second, it changes the default of `buildSandboxPerVisit` back to `false` therefore sharing a single sandbox for many `visit` invocations.
rwjblue
force-pushed
the
make-single-sandbox-per-request-optional
branch
from
d5ebdf0
to
8b953b5
Compare
| let app = shouldBuildApp ? await this.buildApp() : this._applicationInstance; | ||
|
|
||
| if (buildSandboxPerVisit) { | ||
| // entangle the specific application instance to the result, so it can be | ||
| // destroyed when result._destroy() is called (after the visit is | ||
| // completed) | ||
| result.applicationInstance = app; | ||
| } else { | ||
| // save the created application instance so that we can clean it up when | ||
| // this instance of `src/ember-app.js` is destroyed (e.g. reload) | ||
| this._applicationInstance = app; | ||
| } |
There was a problem hiding this comment.
I think this is a bit confusing in terms of variable naming. IIUC, we are deciding here whether to build a new EmberApp every request or re-use the old one and still create a new EmberAppInstance. Please correct me if I am wrong.
If that is the case, shouldn't we call it this._application and result.application. It isn't an instance per say. I know the already existing code calls it appInstance and appInstanceInstance` but that can easily cause confusion :)
There was a problem hiding this comment.
I think this is a bit confusing in terms of variable naming.
Yeah, I don't disagree, but we are talking about instances of Ember.Application and Ember.ApplicationInstance. We have to call them something...
It isn't an instance per say.
Well, it is actually. It is the result of Ember.Application.create(), it is an Ember.Application instance (hence the name this._applicationInstance). The other thing we get is an Ember.ApplicationInstance instance (and is called result.applicationInstanceInstance).
tldr; I erred on the side of "correct but overly verbose", I could change it to .application and .instance if you prefer.
There was a problem hiding this comment.
yeah my only concern was around accidentally misread of the variables due to repeated subwords.
There was a problem hiding this comment.
Yeah :( including Instance in the name of a class is bad news bears 😭
There was a problem hiding this comment.
why not use app as variable name? I agree using appInstance as a variable name is a bit confusing while reading the code :(
There was a problem hiding this comment.
“App” is significantly overused as well. Is it an instance of src/ember-app.js in this project? Is it an instance of lib/broccoli/ember-app.js in ember-cli? Is it an Ember.Application instance? Is it the conceptual “application” that we are rendering? 🙀
Tbh, I’d rather make up completely fabricated names (that don’t use “instance” at all) and link to a naming doc to explain what each of the names mean.
There was a problem hiding this comment.
I agree, it's a tough problem.
This PR does a few things:
Firstly, it allows the end consumer to decide if they want a sandbox per visit or to share a single sandbox over all visits.
Second, it changes the default of
buildSandboxPerVisitback tofalsetherefore sharing a single sandbox for manyvisitinvocations.Reverts the defaults change introduced in #236.