fix(runtime-html): local dependencies for local element #1375

Sayan751 · 2022-03-29T20:28:29Z

Pull Request

📖 Description

It adds a flag for local element and for local elements falls back to parent container if the current container cannot find the resource.

🎫 Issues

Closes #1373

👩‍💻 Reviewer Notes

📑 Test Plan

⏭ Next Steps

It adds a flag for local element and for local elements falls back to parent container if the current container cannot find the resource. #1373

codecov · 2022-03-29T20:29:25Z

Codecov Report

Merging #1375 (8ccfc64) into master (cf2909a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1375   +/-   ##
=======================================
  Coverage   87.89%   87.89%           
=======================================
  Files         171      171           
  Lines       14917    14925    +8     
  Branches     3207     3210    +3     
=======================================
+ Hits        13111    13119    +8     
  Misses       1806     1806

Impacted Files	Coverage Δ
packages/runtime-html/src/template-compiler.ts	`98.35% <100.00%> (+0.02%)`	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

packages/runtime-html/src/template-compiler.ts

…adata

…al dependencies

bigopon · 2022-04-05T09:58:37Z

I think the fix looks appropriate, though it's a bit uncomfortable just to poke the parent container "randomly" like this. And the PR is lacking tests for spreading to local element, I just realized. I'll be having a look at this for a bit.

bigopon · 2022-04-05T12:22:02Z

I think there's another interesting case: local element using owning element:

// my-element
<template as-custom-element="my-local-element">
  <my-element>
</template>

This is not covered at the moment. Do we support this?

Sayan751 · 2022-04-05T16:38:05Z

I think there's another interesting case: local element using owning element:
// my-element
<template as-custom-element="my-local-element">
  <my-element>
</template>
This is not covered at the moment. Do we support this?

Great catch! Need to avoid that. Thanks @bigopon.

Sayan751 · 2022-04-05T17:02:07Z

I think there's another interesting case: local element using owning element:
// my-element
<template as-custom-element="my-local-element">
  <my-element>
</template>
This is not covered at the moment. Do we support this?

@bigopon pushed a fix for this issue.

bit uncomfortable just to poke the parent container "randomly" like this

It is not "random" though. Either you register your custom elements globally (in which case the first attempt at finding the custom element will work) or you register the custom elements locally as dependencies of individual custom elements (I am referring to CustomElementDefinition#dependencies). So when dealing with local elements, it makes sense to fall back on the parent container to resolve/find the custom elements, because in that the parent container would that one from the parent custom element.

And the PR is lacking tests for spreading to local element

Can you please elaborate this part?

bigopon · 2022-04-05T22:41:35Z

The "random" word might have been off. I wanted to express a concern related to relying on container hierarchy where it's not supposed to. Resources has a different semantic in the context of a container hierarchy. So walking to the parent, while being correct in this case, could set a trap that we may walk into later.

And the PR is lacking tests for spreading to local element

Can you please elaborate this part?

I was thinking that if it can recognizes custom element, then it should be able to compile a spread instruction against that custom element usages. Though the API is quite isolated and doesn't walk, so it's probably fine either way.

With regards to the issue, I think there could be another way to fix that may not require us to deal with uncertainty (as in in applications, there may be a custom template controller that adds a container into its hierarchy).

…c/local-template-dependency-issue

…on instead

bigopon · 2022-04-06T05:19:17Z

@Sayan751 I've pushed a commit, in which we build proper dependencies for local elements during the construction of their definitions/classes instead of delaying dependency retrieval until future. This is quite safe because the local element is in complete control of the template compiler, and not exposed anywhere.

A nice side effect of this is we can enable the use of owning element in the local element, without much hiccup.

Sayan751 · 2022-04-06T05:38:34Z

packages/runtime-html/src/template-compiler.ts

+    let i = 0;
+    const ii = localElTypes.length;
+    for (; ii > i; ++i) {
+      (CustomElement.getDefinition(localElTypes[i]).dependencies as Key[]).push(


Nice! I have not though about that. Great solution. I thought that Container will complain about the duplicate registrations.

It makes sense to put/compute the deps array outside the loop as it is not dependent on the iterations of the loop.

when we have more than 1 local element, it's ideal that all local elements can "see" each other. In order to have that, the local elements should have their dependencies built after the owning element has collected all the local elements. Though great poke, I didn't have a test for this case.

I've added another test to assert the behavior/expectation here https://github.com/aurelia/aurelia/pull/1375/files#diff-24a8cd6d83b4df0282eacd3f1df4c95ee8e4a93d84c42d527604802a7b6f0e20R2252

I was referring to this part:

...context.def.dependencies ?? emptyArray, ...context.deps ?? emptyArray,

This can be extracted right before this loop (L1523), right?

ahh yes thanks, I thought there was only one loop, while I actually wrote this 2nd loop lol

Sayan751 · 2022-04-06T05:39:56Z

packages/__tests__/3-runtime-html/template-compiler.spec.ts

+    `.trim();
+    @customElement({ name: 'my-app', template })
+    class App {
+      public prop = false;


I think when you set this to true we will have a stack overflow. From that sense, it makes more sense for me to put the previous restriction in place.

there's an if to prevent that in the <my-le/>. I think it's an inherent "rule" that if there is a recursive call, it should be constrained.

Exactly! That's why IMO we should never support this case and throw a compilation error, because my-le my-app will always fail.

It'll be quite unnatural if normal custom element can do that, and local element cannot. I think we can add a warning though, in dev mode.

…ecognition

…c/local-template-dependency-issue

bigopon · 2022-04-06T07:45:49Z

Thanks @Sayan751 , the fix is in.

Sayan751 · 2022-04-06T08:08:54Z

Thanks @Sayan751 , the fix is in.

Thank you @bigopon for jumping in!!

fix(runtime-html): local dependencies for local element

de75c4c

It adds a flag for local element and for local elements falls back to parent container if the current container cannot find the resource. #1373

Sayan751 requested a review from bigopon March 29, 2022 20:28

bigopon added this to the v2.0-beta milestone Mar 31, 2022

bigopon reviewed Mar 31, 2022

View reviewed changes

packages/runtime-html/src/template-compiler.ts Outdated Show resolved Hide resolved

Sayan751 and others added 4 commits April 2, 2022 08:53

chore(runtime-html): removed the isLocalElemeent flag in favor of met…

31341c7

…adata

refactor(template-compiler): minor changes

17a5fce

test(template-compiler): template-compiler + local-element + non-glob…

48168f5

…al dependencies

Merge branch 'master' into topic/local-template-dependency-issue

b9f140c

chore(local-template): prevented using parent in the local template

a168cbf

bigopon added 2 commits April 6, 2022 15:16

Merge branch 'master' of https://github.com/aurelia/aurelia into topi…

eaf5234

…c/local-template-dependency-issue

refactor(local-template): register dependency while building definiti…

119a217

…on instead

chore: fix deepscan issue

c2b337e

Sayan751 commented Apr 6, 2022

View reviewed changes

bigopon added 2 commits April 6, 2022 16:53

chore(local-element): add another test for hoisting & cross element r…

a9d0567

…ecognition

Merge branch 'master' of https://github.com/aurelia/aurelia into topi…

8ccfc64

…c/local-template-dependency-issue

bigopon merged commit 0d48dbf into master Apr 6, 2022

bigopon deleted the topic/local-template-dependency-issue branch April 6, 2022 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(runtime-html): local dependencies for local element #1375

fix(runtime-html): local dependencies for local element #1375

Sayan751 commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

bigopon commented Apr 5, 2022

bigopon commented Apr 5, 2022

Sayan751 commented Apr 5, 2022 •

edited

Loading

Sayan751 commented Apr 5, 2022

bigopon commented Apr 5, 2022

bigopon commented Apr 6, 2022 •

edited

Loading

Sayan751 Apr 6, 2022

bigopon Apr 6, 2022

bigopon Apr 6, 2022

Sayan751 Apr 6, 2022

bigopon Apr 6, 2022

Sayan751 Apr 6, 2022

bigopon Apr 6, 2022

Sayan751 Apr 6, 2022

bigopon Apr 6, 2022

bigopon commented Apr 6, 2022

Sayan751 commented Apr 6, 2022

fix(runtime-html): local dependencies for local element #1375

fix(runtime-html): local dependencies for local element #1375

Conversation

Sayan751 commented Mar 29, 2022 • edited Loading

Pull Request

📖 Description

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

⏭ Next Steps

codecov bot commented Mar 29, 2022 • edited Loading

Codecov Report

bigopon commented Apr 5, 2022

bigopon commented Apr 5, 2022

Sayan751 commented Apr 5, 2022 • edited Loading

Sayan751 commented Apr 5, 2022

bigopon commented Apr 5, 2022

bigopon commented Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigopon commented Apr 6, 2022

Sayan751 commented Apr 6, 2022

Sayan751 commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

Sayan751 commented Apr 5, 2022 •

edited

Loading

bigopon commented Apr 6, 2022 •

edited

Loading