feat: Add support for locators #701
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
reviewed
Oct 3, 2018
| */ | ||
| var locatorDetail = e.detail; | ||
| var target = locatorDetail.target; | ||
| var host = locatorDetail.host; |
caridy
reviewed
Oct 3, 2018
| } | ||
| */ | ||
| var locatorDetail = e.detail; | ||
| var target = locatorDetail.target; |
There was a problem hiding this comment.
what is this? how is this different from e.target?
caridy
suggested changes
Oct 3, 2018
caridy
reviewed
Oct 3, 2018
| const host: Element = component[ViewModelReflection].elm as Element; | ||
| const locatorEvent = new CustomEvent('LWCLocator', { | ||
| bubbles: false, | ||
| detail: { |
There was a problem hiding this comment.
detail should contain only strings. extract that information from the vm that owns the currentTarget.
caridy
reviewed
Oct 3, 2018
|
|
||
| return function(event: Event) { | ||
| try { | ||
| const target: Element = event.currentTarget as Element; |
There was a problem hiding this comment.
this currentTarget should be extracted using a getter, can't use the dot notation.
There was a problem hiding this comment.
How do I get the VNode for the target element here ?
tariqrafique
commented
Oct 11, 2018
| @@ -39,8 +39,8 @@ describe('fixtures', () => { | |||
| const src = readFixtureFile('actual.html'); | |||
|
|
|||
| const configOverride = JSON.parse(readFixtureFile('config.json')); | |||
| const expectedCode = readFixtureFile(expectedJsFile); | |||
| const expectedMetaData = JSON.parse(readFixtureFile(expectedMetaFile)); | |||
| let expectedCode = readFixtureFile(expectedJsFile); | |||
There was a problem hiding this comment.
these are mutated in the case where the expected js and metadata files don't exists. May as well let the test run properly and use the same value for these thats being written to file
tariqrafique
commented
Oct 11, 2018
| } | ||
|
|
||
| if (expectedMetaData === null) { | ||
| // write metadata file if doesn't exist (ie new fixture) | ||
| const metadata = { | ||
| warnings: actual.warnings, | ||
| ...actualMeta, | ||
| metadata: {...actualMeta}, |
There was a problem hiding this comment.
this was actually busted. the file would not be correctly written out
Benchmark resultsBase commit: lwc-engine-benchmark
|
tariqrafique
commented
Oct 11, 2018
docs/proposals/locators.md
Outdated
| @@ -0,0 +1,349 @@ | |||
| # RFC: Locators | |||
There was a problem hiding this comment.
I still need to update the RFC. Please focus on changes to the compiler and engine and test cases.
There was a problem hiding this comment.
@tariq-sfdc if you don't have time to finish this today/tomorrow, then remove it from the PR, and make another PR just for the RFC, no rush.
tariqrafique
commented
Oct 11, 2018
caridy
reviewed
Oct 12, 2018
caridy
reviewed
Oct 12, 2018
| @@ -17,6 +18,8 @@ export interface RenderAPI { | |||
| p(text: string): VComment; | |||
| d(value: any): VNode | null; | |||
| b(fn: EventListener): EventListener; | |||
| fb(fn: () => any): () => any; | |||
| ll(fn: EventListener): EventListener; | |||
There was a problem hiding this comment.
signature does not correspond to the implementation.
caridy
reviewed
Oct 12, 2018
| @@ -438,6 +441,53 @@ export function b(fn: EventListener): EventListener { | |||
| }; | |||
| } | |||
|
|
|||
| // [f]unction_[b]ind | |||
| export function fb(fn: () => any): () => any { | |||
There was a problem hiding this comment.
I don't think this is what we need. What we need is just a regular binding mechanism where you pass the obj and the fn to be bound, and this method does the trick. Imagine something like this: <button locator:context={foo.bar}>, in that case, bar method must be called with the this value set to $cmp.foo. Keep in mind that invokeComponentCallback is for callbacks, e.g.: connectedCallback, etc, it is not for common functions like locator:context
There was a problem hiding this comment.
so looking at what the click event handler does is api_bind and cmp is set as the context. Is that intended behavior ?
if todoItem is the iteration index and todoItem.handleClick was the event handler attached to a todo item, then when the click handler executes (no locators), the this context is set to the component. You don't get access to the this which is the todo items
So we're saying that for locator:provider it's going to be different ?
There was a problem hiding this comment.
a context provider is not an event handler. api.b is equivalent to elm.addEventListener('click', foo.bar), in which case the context is still the element. In your case, it is not an event handler, it is just a function call.
There was a problem hiding this comment.
got it.
So if we have {x.y.z}, then we first find to $cmp if we need to or if it's inside an iteration then that remains unchanged.
Then from the resulting expression, the this context should be everything just before .z ?
Is there anything that already in codegen that can help extract this ?
There was a problem hiding this comment.
yes, that's it.
no, I don't know.
caridy
reviewed
Oct 12, 2018
| locator.resolved = { | ||
| target: id, | ||
| host: locator.id, | ||
| targetProvider: isFunction(provider) && invokeComponentCallback(vm, provider), |
There was a problem hiding this comment.
here is should just call provider(). when is provider supposed to be a non-function? in the method signature, it says it must be a function, why the test?
caridy
reviewed
Oct 12, 2018
| target: id, | ||
| host: locator.id, | ||
| targetProvider: isFunction(provider) && invokeComponentCallback(vm, provider), | ||
| hostProvider: isFunction(locator.provider) && locator.provider() |
There was a problem hiding this comment.
locator.provider.call() otherwise you might be leaking information as a this value.
There was a problem hiding this comment.
when locator.provider will not be a function? if locator exists, how is the provider not provided?
caridy
reviewed
Oct 12, 2018
| const interaction = { | ||
| target: target, | ||
| scope: host, | ||
| context: Object.assign(targetProvider || {}, hostProvider) |
There was a problem hiding this comment.
maybe this work could be done in the layer above inside ll
caridy
reviewed
Oct 12, 2018
| scope: host, | ||
| context: Object.assign(targetProvider || {}, hostProvider) | ||
| }; | ||
| console.log("interaction", interaction); |
caridy
reviewed
Oct 12, 2018
| ($ctx._m1 = locator_listener_bind( | ||
| $cmp.handleClick, | ||
| "button-in-slot", | ||
| $cmp.locatorProvider |
There was a problem hiding this comment.
remember that this one should also be bounded via function_bind, something like function_bind($cmp, $cmp.locatorProvider)
caridy
reviewed
Oct 12, 2018
| locator: { | ||
| id: "button-in-slot", | ||
| provider: | ||
| _m0 || ($ctx._m0 = function_bind($cmp.locatorProvider)) |
There was a problem hiding this comment.
function_bind($cmp, $cmp.locatorProvider) should be the proper process to bind the method.
There was a problem hiding this comment.
make sure that it also work for deep obj as a provider, e.g: function_bind($cmp.foo, $cmp.foo.bar)
caridy
suggested changes
Oct 12, 2018
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
reviewed
Oct 16, 2018
| @@ -9,19 +9,21 @@ import { VM } from "./vm"; | |||
| type ServiceCallback = (component: object, data: VNodeData, def: ComponentDef, context: Context) => void; | |||
| interface ServiceDef { | |||
| wiring?: ServiceCallback; | |||
| located?: ServiceCallback; | |||
There was a problem hiding this comment.
we should probably rename this to locator as well
caridy
reviewed
Oct 16, 2018
| locator.resolved = { | ||
| target : id, | ||
| host : locator.id, | ||
| targetProvider: provider && provider(), |
There was a problem hiding this comment.
isFunction(provider) ? provider() : {} maybe?
same for the following line.
Benchmark resultsBase commit: lwc-engine-benchmark
|
tariqrafique
commented
Oct 19, 2018
| @@ -438,6 +441,54 @@ export function b(fn: EventListener): EventListener { | |||
| }; | |||
| } | |||
|
|
|||
| // [f]unction_[b]ind | |||
| export function fb(scope: any, fn: (...args: any[]) => any): () => any { | |||
There was a problem hiding this comment.
@caridy I'll remove this completely and instead use invokeComponentCallback ?
tariqrafique
commented
Oct 19, 2018
| context: { | ||
| locator: { | ||
| id: "todo-item", | ||
| provider: function_bind(todo, todo.locatorProviderTodo) |
There was a problem hiding this comment.
this will just become
provider:api_bind(todo.locatorProviderTodo) // memorized
tariqrafique
commented
Oct 19, 2018
| click: locator_listener_bind( | ||
| todo.clickHandler, | ||
| "todo-item", | ||
| function_bind(todo, todo.locatorProviderTodo) |
There was a problem hiding this comment.
same here. it just becomes
todo.locatorProviderTodo
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
reviewed
Oct 23, 2018
caridy
reviewed
Oct 23, 2018
caridy
reviewed
Oct 23, 2018
caridy
reviewed
Oct 23, 2018
caridy
approved these changes
Oct 23, 2018
Benchmark resultsBase commit: lwc-engine-benchmark
|
Test needs to run in IE11. Use ES5 function definition style
Benchmark resultsBase commit: lwc-engine-benchmark
|
|
@tariq-sfdc thanks a lot of all the work. Let's get this merge today. |
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.
Add support to the compiler and engine for locators.
Details
See the locators.md proposal in the file list for this PR.
Does this PR introduce a breaking change?