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

Disallow usages of innerHTML for no-container rule #889

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

CodingItWrong
Copy link
Contributor

Checks

Changes

  • So far, updates the rule to reject when innerHTML is accessed on a destructured container proprety
  • QUESTION: one test case is not yet covered: when the return value of a render function is assigned to a render result variable like view, and then it is accessed as view.container.innerHTML. For container methods, this seems to be caught by the code on lines 95-106. But we can't use that code as-is in MemberExpression() because of the error Property 'callee' does not exist on type 'MemberExpression'.. Looking over our code, do you have an ideas on how we could detect the view.container.innerHTML case in MemberExpression()? @Belco90

Context

Fixes #883

// messageId: 'noContainer',
// },
// ],
// },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Belco90 This comment-out test is the test that is failing now, that shows us that we are not yet handling the view.container.innerHTML case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I need to go again through the utils for navigating the nodes, I think we had something to fix the problem described in the main description.

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.21%. Comparing base (93a6ab9) to head (514ef11).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #889      +/-   ##
==========================================
- Coverage   96.23%   96.21%   -0.03%     
==========================================
  Files          44       44              
  Lines        2419     2432      +13     
  Branches     1000     1007       +7     
==========================================
+ Hits         2328     2340      +12     
- Misses         91       92       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MichaelDeBoey
Copy link
Member

@CodingItWrong Are you still interested in implementing this feature?

If so, can you please rebase onto latest main?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail no-container on use of innerHTML
4 participants