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

account for cases when parentNode and nextNode are undefined #1128

Merged
merged 4 commits into from
Jan 15, 2021

Conversation

patrickkettner
Copy link
Collaborator

fix for #1121

Copy link
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

Can we test that non of the transformers fails when skipNodeAndChildren returns null?

let err = null;

try {
await treeParser.parse(`<!doctype html>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this test ever fail?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it would fail if the code I added isn't there

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, interesting. skipNodeAndChildren is only called from within transformers and these shouldn't be run when calling treeParser.parse.

@patrickkettner
Copy link
Collaborator Author

Can we test that non of the transformers fails when skipNodeAndChildren returns null?

will do

@patrickkettner
Copy link
Collaborator Author

@sebastianbenz I have gotten stuck on the tests. I am trying to reuse the same looped abstraction to test all of the transformers, and just unregister, mock, and reregister. But I am getting lost in jest - I pushed my current (broken) state, would you be able to take a look?

getDirectories(testConfig.testDir).forEach((testDir) => {
it(basename(testDir), (done) => runTests(testDir, done));

it('works when skipNodeAndChildren returns null', function (done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer not to add more tests here. Instead, we could add an end-to-end test here that runs a document with a template as last element through all transformers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good

let err = null;

try {
await treeParser.parse(`<!doctype html>
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, interesting. skipNodeAndChildren is only called from within transformers and these shouldn't be run when calling treeParser.parse.

Copy link
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -42,6 +42,7 @@
"htmlparser2": "5.0.1",
"https-proxy-agent": "5.0.0",
"lru-cache": "6.0.0",
"mock-require": "^3.0.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need this?

@sebastianbenz
Copy link
Collaborator

Can you revert the package lock to main.

@sebastianbenz sebastianbenz merged commit 9a40373 into main Jan 15, 2021
@sebastianbenz
Copy link
Collaborator

Thanks Patrick! The CI failures were unrelated to this PR.

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

Successfully merging this pull request may close these issues.

None yet

2 participants