Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_analyze): useValidAnchor considering all possible expressions #3599

Merged
merged 4 commits into from
Nov 9, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Nov 8, 2022

Summary

Closes #3590.

We need to accept every expression that can potentially return a string.

Test Plan

> cargo test -p rome_js_analyze -- use_valid_anchor

@netlify
Copy link

netlify bot commented Nov 8, 2022

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit f98920e
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/636bd8102c4d330008e1c485

@xunilrj xunilrj temporarily deployed to netlify-playground November 8, 2022 18:36 Inactive
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

@xunilrj xunilrj temporarily deployed to netlify-playground November 9, 2022 10:57 Inactive
@xunilrj xunilrj marked this pull request as ready for review November 9, 2022 10:58
@xunilrj xunilrj requested a review from leops as a code owner November 9, 2022 10:58
@calibre-analytics
Copy link

calibre-analytics bot commented Nov 9, 2022

Comparing fix(rome_js_analyze): useValidAnchor considering all possible expressions Snapshot #6 to median since last deploy of rome.tools.

LCP? CLS? TBT?
Overall
Median across all pages and test profiles
2.54s
from 179ms
0.0
no change
238ms
from 12ms
Chrome Desktop
Chrome Desktop • Cable
2.54s
from 179ms
0.0
no change
420ms
from 65ms
iPhone, 4G LTE
iPhone 12 • 4G LTE
1.05s
from 143ms
0.0
no change
17ms
no change
Motorola Moto G Power, 3G connection
Motorola Moto G Power • Regular 3G
16.5s
from 506ms
0.0
no change
238ms
from 12ms

1 page tested

 Home

Browser previews

Chrome Desktop iPhone, 4G LTE Motorola Moto G Power, 3G connection
Chrome Desktop iPhone, 4G LTE Motorola Moto G Power, 3G connection

Most significant changes

Value Budget
JS Parse & Compile
iPhone, 4G LTE
470ms
from 10ms
Largest Contentful Paint
Motorola Moto G Power, 3G connection
16.5s
from 506ms
First Contentful Paint
Motorola Moto G Power, 3G connection
16.5s
from 506ms
Total Blocking Time
Motorola Moto G Power, 3G connection
238ms
from 12ms
Total JavaScript Size in Bytes
Chrome Desktop
5.27 MB
from 309 KB

28 other significant changes: Total JavaScript Size in Bytes on iPhone, 4G LTE, Total JavaScript Size in Bytes on Motorola Moto G Power, 3G connection, JS Parse & Compile on Motorola Moto G Power, 3G connection, First Contentful Paint on Chrome Desktop, Largest Contentful Paint on Chrome Desktop, JS Parse & Compile on Chrome Desktop, Speed Index on Motorola Moto G Power, 3G connection, First Contentful Paint on iPhone, 4G LTE, Largest Contentful Paint on iPhone, 4G LTE, Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, Number of Requests on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection, Time to Interactive on Chrome Desktop, Total Blocking Time on Chrome Desktop, Time to Interactive on Motorola Moto G Power, 3G connection, Time to Interactive on iPhone, 4G LTE, Speed Index on Chrome Desktop, Total HTML Size in Bytes on Chrome Desktop, Total HTML Size in Bytes on iPhone, 4G LTE, Total HTML Size in Bytes on Motorola Moto G Power, 3G connection, Total Image Size in Bytes on Chrome Desktop, Total Image Size in Bytes on iPhone, 4G LTE, Total Image Size in Bytes on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Chrome Desktop

Calibre: Site dashboard | View this PR | Edit settings | View documentation

@xunilrj xunilrj temporarily deployed to netlify-playground November 9, 2022 11:06 Inactive
| JsAnyExpression::JsSuperExpression(_)
| JsAnyExpression::JsUnaryExpression(_)
| JsAnyExpression::JsUnknownExpression(_)
| JsAnyExpression::JsYieldExpression(_)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would certainly be a very awkward way to write a component, but technically a yield expression may return a valid string value for href

Copy link
Contributor

Choose a reason for hiding this comment

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

In my view, we should either both list JsAwaitExpression and JsYieldExpression or neither of them. Because both can yield a valid URL but only if the framework supports async execution when rendering.

@leops leops added this to the 10.0.1 milestone Nov 9, 2022
| JsAnyExpression::JsSuperExpression(_)
| JsAnyExpression::JsUnaryExpression(_)
| JsAnyExpression::JsUnknownExpression(_)
| JsAnyExpression::JsYieldExpression(_)
Copy link
Contributor

Choose a reason for hiding this comment

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

In my view, we should either both list JsAwaitExpression and JsYieldExpression or neither of them. Because both can yield a valid URL but only if the framework supports async execution when rendering.

@xunilrj xunilrj temporarily deployed to netlify-playground November 9, 2022 16:40 Inactive
@xunilrj xunilrj merged commit 8e03ce7 into main Nov 9, 2022
@xunilrj xunilrj deleted the fix/useValidAnchor-not-considering-all-expressions branch November 9, 2022 16:56
jeysal added a commit to jeysal/rometools that referenced this pull request Nov 10, 2022
* upstream/main:
  fix(npm/js-api): Import type from @rometools/backend-jsonrpc (rome#3647)
  doc(npm/js-api): Add experimental warning to README
  fix(npm/js-api): Lazy load backend implementations (rome#3652)
  feat(rome_lsp): stop the daemon after the last client disconnects (rome#3643)
  fix(npm/js_api): Ensure JS API build contains `dist` folder (rome#3648)
  fix(rome_cli): ensures the service only connects to compatible versions (rome#3642)
  feat(playground): add settings button and group IR (rome#3559)
  release: v10.0.1 (rome#3649)
  fix(rome_js_analyze): False positives for interactive elements in `useKeyWithClickEvents` (rome#3644)
  fix(rome_js_semantic): support for object and array bindings when exporting (rome#3620)
  doc(editors): Clarify Rome discovery (rome#3639)
  fix(rome_js_analyze): improve the detection of `ReactDOM.render` calls in `noRenderReturnValue` (rome#3626)
  chore(npm): add license (rome#3629)
  Add rustdocs index
  Add environment
  Change rust docs to use Netlify
  fix(rome_js_analyze): useValidAnchor considering all possible expressions (rome#3599)
  fix(rome_js_formatter): Trailing comma inside import rome#3600 (rome#3624)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive rule lint/a11y/useValidAnchor when calling a function
3 participants