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

fix: querySelector returns null when nothing is found. #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JeanMeche
Copy link
Member

@JeanMeche JeanMeche commented Jul 17, 2023

Also: commenting unused test code that prevented from running the unit tests.

Fixes angular/angular#51068

Copy link

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

Other than the tests being commented out, this looks like it should be fine. Can you restore those tests?

@@ -81,271 +81,271 @@ function read(file) {
return fs.readFileSync(Path.resolve(__dirname, '..', file), 'utf8');
}

var testharness = require(__dirname + '/web-platform-tests/resources/testharness.js');
// var testharness = require(__dirname + '/web-platform-tests/resources/testharness.js');

Choose a reason for hiding this comment

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

Please uncomment this file. While all the tests don't pass, we do have some that we need and rely on.

Copy link
Member Author

@JeanMeche JeanMeche Jul 17, 2023

Choose a reason for hiding this comment

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

I'm probably missing some insights here. I was getting an error because that particular file doesn't exisit on the repo. Hense all the commented code.

Also to me that variable wasn't even used, the tests using the harness function were comment at the end of the file !

@AndrewKushnir
Copy link

FYI, this would be a breaking change, we should make sure we merge it once the main branch is available for v17 changes.

@JeanMeche
Copy link
Member Author

@AndrewKushnir While I get that it feels like a breaking change, I thought It would be more of a fix since we have a different behavior between Browser and SSR.

@AndrewKushnir
Copy link

While I get that it feels like a breaking change, I thought It would be more of a fix since we have a different behavior between Browser and SSR.

@JeanMeche agreed, this is a legit fix, but it'd still be breaking for cases where there is a strict comparison like document.querySelector('my-cmp') !== undefined, so we'd need to avoid breaking this pattern in a minor or a patch release.

@mwyld
Copy link

mwyld commented Feb 15, 2024

Hi Angular team, do you have an update on when you think this issue will be resolved?

Also: commenting unused tests code that prevented running the unit tests.

Fixes angular/angular#51068
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants