[REFACTOR] Extract environment variables into global context #1138
Conversation
This file contains 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
rwjblue
reviewed
Aug 20, 2020
| { | ||
| "name": "@glimmer/global-context", | ||
| "version": "0.58.0", | ||
| "repository": "https://github.com/glimmerjs/glimmer-vm/tree/master/packages/@glimmer/global-context", |
There was a problem hiding this comment.
We should update these (in a future PR) to something like:
{
"repository": {
"type" : "git",
"url" : "https://github.com/glimmerjs/glimmer-vm.git",
"directory": "packages/@glimmer/global-context"
}
}https://docs.npmjs.com/configuring-npm/package-json.html#repository
rwjblue
reviewed
Aug 20, 2020
There was a problem hiding this comment.
- Should we move the global revision stuff here? It seems very much in the same vein, though maybe we don't want to have to call a method to bump so might not want to move it...
- Should we add a validation similar to the one we added to
@glimmer/validationsto prevent loading the module two times? Seems like@glimmer/global-contexthas much the same constraints as that one does... - Can you do a benchmark to confirm this doesn't have problems? (I suspect it will actually be somewhat beneficial, but good to confirm)
pzuraq
force-pushed
the
refactor/make-env-module-state
branch
from
910635e to
79ff403
Compare
rwjblue
reviewed
Aug 21, 2020
| valueForTag(tag1); | ||
| dirtyTag(tag3); | ||
| valueForTag(tag1); | ||
| // let tag1 = createUpdatableTag(); |
There was a problem hiding this comment.
These comments should either be brought back or deleted.
There are a number of environment variables that are currently threaded through glimmer that are: 1. Only environment variables for compatibility reasons (e.g. legacy Ember compat) 2. Never expected to change over the lifetime of an append 3. Never expected to differ between different apps or render trees in the same context. This PR consolidates these into a single global context, exported from `@glimmer/global-context`, which is set once by the embedding environment before rendering. It also upstreams the logic for `protocolForUrl`, which was not specific to Ember, and simplifies the `attributeFor` logic to be as minimal as possible an interface for Ember.
pzuraq
force-pushed
the
refactor/make-env-module-state
branch
from
e9c5581 to
47142cc
Compare
pzuraq
pushed a commit
to emberjs/ember.js
that referenced
this pull request
Aug 22, 2020
|
Benchmark shows no significant change: |
rwjblue
approved these changes
Aug 22, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
There are a number of environment variables that are currently threaded
through glimmer that are:
Ember compat)
the same context.
This PR consolidates these into a single global context, exported from
@glimmer/global-context, which is set once by the embeddingenvironment before rendering. It also upstreams the logic for
protocolForUrl, which was not specific to Ember, and simplifiesthe
attributeForlogic to be as minimal as possible an interface forEmber.