fix(engine): closes #515 by removing lazy init #517
Merged
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
caridy
commented
Jul 18, 2018
| @@ -1,5 +1,5 @@ | |||
| import { LightningElement, getHostShadowRoot } from "../../framework/html-element"; | |||
There was a problem hiding this comment.
this one still exhibit the circular issue, but changing the order fixes it for me.
There was a problem hiding this comment.
I would like to talk about this. If the import order mater then we still have a circular dependency issue.
There was a problem hiding this comment.
Just a suggestion: We should use a tool(for example: https://github.com/pahen/madge) to generate a dependency graph of the modules and remove any cyclic dependencies.
caridy
commented
Jul 18, 2018
| @@ -399,58 +395,54 @@ const globalElmDescriptors: PropertyDescriptorMap = create(null, { | |||
| [PatchedFlag]: {} | |||
| }); | |||
|
|
|||
| let globalInitialization: any = () => { | |||
There was a problem hiding this comment.
removing the method to just run the code directly during evaluation instead of lazily.
Benchmark resultsBase commit: lwc-engine-benchmark
|
pmdartus
reviewed
Jul 18, 2018
There was a problem hiding this comment.
Can we also get rid of this import? https://github.com/salesforce/lwc/blob/master/packages/lwc-engine/src/framework/modules/__tests__/props.spec.ts#L1
| @@ -1,5 +1,5 @@ | |||
| import { LightningElement, getHostShadowRoot } from "../../framework/html-element"; | |||
There was a problem hiding this comment.
I would like to talk about this. If the import order mater then we still have a circular dependency issue.
|
|
||
| if (process.env.NODE_ENV !== 'production') { | ||
| patchLightningElementPrototypeWithRestrictions(BaseElement.prototype); | ||
| forEach.call(ElementAOMPropertyNames, (propName: string) => { |
There was a problem hiding this comment.
@caridy I assume there a reason for putting this in def.ts instead of html-element.ts
There was a problem hiding this comment.
because this generates two sets, the set that goes into the component proto, and the set that goes into the element proto.
caridy
force-pushed
the
caridy/fix-issue-515-lazy-initialization-locker
branch
from
3d7767e to
a6e3aa7
Compare
ekashida
approved these changes
Aug 17, 2018
Benchmark resultsBase commit: lwc-engine-benchmark
|
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.
Details
Removes the lazy initialization now that the circular dependencies are not longer an issue after the separation of framework from faux-shadow.
This fixes #515
Does this PR introduce a breaking change?