+
Skip to content

Conversation

Ponynjaa
Copy link
Collaborator

Fixed locator not being true by default when using any other options.
Closes #802

@Ponynjaa
Copy link
Collaborator Author

Ponynjaa commented Dec 11, 2024

Hi @karfau ,
I'm a little surprised about the test that's now failing... it seems like this was the intended behavior before my change... however why is it documented that the default value of this option is true, when it sometimes is not? This seems like a mistake for me...

I think it should always be true whenever this exact option isn't given, but I understand that this probably could be a breaking change now...

What do you think?

Copy link
Member

@karfau karfau left a comment

Choose a reason for hiding this comment

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

Thank you for reporting and providing a fix.
I think the bug report and fix make sense.
We just need to change the expectation in the failing test.

function DOMParser(options) {
options = options || { locator: true };
options = options || {};
if (typeof options.locator === 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

How is this different from

Suggested change
if (typeof options.locator === 'undefined') {
if (options.locator === undefined) {

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case it doesn't make any difference, so yeah I'll change that

@karfau
Copy link
Member

karfau commented Dec 12, 2024

Regarding the breaking change, I would say this was more an oversight. And the way the docs are describing it makes more sense.
And since it is already the default when no options are passed and it is easy to explicitly turn it of if you already have options i would publish it as a patch change.

Copy link

codecov bot commented Dec 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.06%. Comparing base (07502cd) to head (0709a82).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #803   +/-   ##
=======================================
  Coverage   95.06%   95.06%           
=======================================
  Files           8        8           
  Lines        2167     2169    +2     
  Branches      570      571    +1     
=======================================
+ Hits         2060     2062    +2     
  Misses        107      107           

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

@karfau karfau merged commit 9f90a0a into master Dec 14, 2024
38 checks passed
@karfau karfau deleted the fix-locator branch December 14, 2024 08:02
@karfau karfau added this to the 0.9.7 milestone Jan 19, 2025
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.

locator isn't true by default when using any other options

2 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载