Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: detect dangling properties on custom elements #2864

Conversation

pkozlowski-opensource
Copy link
Member

@mhevery @tbosch this is an attempt to query web component properties at run-time to detect invalid property bindings. I've managed to make it work in JS, but it would need more effort in TS / Dart (still, should be manageable).

But before spending more time on it, I would like to validate the approach with you: essentially we are creating an instance of a custom element if:

  • we are in a browser that supports custom elements
  • name of a tag looks like a custom element (contains -)

As soon as we've got an instance of a custom element we can query its properties. This has an advantage over static / hand-written "schema", since people don't have to write it by hand and valid properties can be discovered at run-time. Not sure about the potential perf impact (we can easily cache results of those checks, this would be roughly equivalent of maintaing static schema in memory). I also need to check if there are no side effect for elements that are created by not attached to the DOM.

@pkozlowski-opensource pkozlowski-opensource added state: WIP action: discuss action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 3, 2015
@mhevery mhevery added pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 6, 2015
@mhevery
Copy link
Contributor

mhevery commented Jul 6, 2015

I think this is a great approach. The more we can extract at runtime the better.

I still think that there will be a need for a schema:

  1. it is needed for events,
  2. it is needed for SVG,
  3. it is needed for matching names such as inner-html vs innerHTML
  4. it is needed for web-components which may not correctly pre-initialize the properties.

But I think we can leave the schema for later now that 90% use case has been covered.

DO: cache the results for speed.

@pkozlowski-opensource
Copy link
Member Author

@mhevery totally agree that we will need some hand-written / "static" schema for things that we can't infer at runtime. But I'm happy to figure as much as needed at runtime as possible so people don't have to write it "by hand".

I'm going to clean-up this PR and merge - then we can review all the cases that are left.

Thnx for the review!

@mhevery
Copy link
Contributor

mhevery commented Jul 6, 2015

Sounds good to me.

@pkozlowski-opensource
Copy link
Member Author

Status update: I'm back from the AngularJS trainings I've been running - will rebase this PR and add Dart support today / tomorrow (Mon / Tue)

@pkozlowski-opensource pkozlowski-opensource force-pushed the web_components_props branch 3 times, most recently from 8ecf200 to 5e2fc5e Compare July 20, 2015 15:07
@pkozlowski-opensource
Copy link
Member Author

@mhevery @tbosch I've added caching, rebased and made the test pass. The Dart impl is still pending as I've hitting some issues (need to check a couple of things and will ask for help if blocked).

I wonder if we could land the JS support for now as I don't want this stuff to bit-rotten to much. I can also see that there is work done by @tbosch on the props checking in Dart, so it would be good to verify if my changes are compatible here.

@naomiblack naomiblack added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: discuss pr_state: LGTM labels Jul 20, 2015
@naomiblack naomiblack added this to the alpha-32 milestone Jul 20, 2015
@mhevery
Copy link
Contributor

mhevery commented Jul 20, 2015

@pkozlowski-opensource I am fine with landing the JS support and backfilling the Dart later (provided that it does not break dart in the interim)

@mhevery mhevery added pr_state: LGTM action: merge The PR is ready for merge by the caretaker labels Jul 20, 2015
@mhevery
Copy link
Contributor

mhevery commented Jul 20, 2015

LGTM

@tbosch
Copy link
Contributor

tbosch commented Jul 20, 2015

Sorry, missed the messaging on this thread.
When we run Dart transformers, we can't create nor check custom elements, as we are not running in a browser. For them, we actually need a hand written schema...

@tbosch
Copy link
Contributor

tbosch commented Jul 21, 2015

What is the status here? Should this be merged or is this WIP?

@pkozlowski-opensource
Copy link
Member Author

@tbosch I'm going to close this one and open a new PR with full schema support.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker action: review The PR is still awaiting reviews from at least one requested reviewer cla: yes state: WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants