Skip to content

refactor: replace NodeSelector with Document in preparation for Typescript upgrade#1065

Merged
dbjorge merged 3 commits intomicrosoft:masterfrom
dbjorge:replace-nodeselector-with-document
Aug 14, 2019
Merged

refactor: replace NodeSelector with Document in preparation for Typescript upgrade#1065
dbjorge merged 3 commits intomicrosoft:masterfrom
dbjorge:replace-nodeselector-with-document

Conversation

@dbjorge
Copy link
Copy Markdown
Contributor

@dbjorge dbjorge commented Aug 13, 2019

Description of changes

Updating from Typescript 2.9 to 3.5 results in a variety of errors that look like this:

ERROR in Q:/repos/accessibility-insights-web/src/common/document-manipulator.ts(4,42):
TS2304: Cannot find name 'NodeSelector'.

It looks like microsoft/TypeScript-DOM-lib-generator#647 was the point where it was removed, merged into a separate ParentNode interface. We considered using ParentNode instead here, but Document seemed closer to the intended usage in most cases and led to the tests working more similarly to the product code than anything else, so that's what I went with.

Replacing the usages with Document is a no-op in Typescript 2.9 (so nice to do as a separate PR, to make the eventual Typescript upgrade PR smaller). It also forces the affected tests to be a little bit better (they create test documents for the cases where the product passes real documents, rather than creating test nodes within a global document and hoping that they act similarly and that no other test touches global document state).

Pull request checklist

  • [n/a] Addresses an existing issue: Fixes #0000
  • Added relevant unit test for your changes. (yarn test)
  • Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • Ran precheckin (yarn precheckin)
  • [n/a] (UI changes only) Added screenshots/GIFs to description above
  • [n/a] (UI changes only) Verified usability with NVDA/JAWS

@Shobhit1
Copy link
Copy Markdown
Contributor

Just out of curiosity and to learn; can you please link the resource or a github issue/document where we can read more about the NodeSelector being replaced by Document in Typescript 3.0 or NodeSelector being replaced in the new version.

@dbjorge
Copy link
Copy Markdown
Contributor Author

dbjorge commented Aug 14, 2019

@Shobhit1:

Just out of curiosity and to learn; can you please link the resource or a github issue/document where we can read more about the NodeSelector being replaced by Document in Typescript 3.0 or NodeSelector being replaced in the new version.

Updated PR description accordingly, good suggestion!

@dbjorge dbjorge marked this pull request as ready for review August 14, 2019 19:52
@dbjorge dbjorge requested a review from a team August 14, 2019 19:52
@waabid
Copy link
Copy Markdown
Member

waabid commented Aug 14, 2019

rather than creating test nodes within a global document and hoping that they act similarly and that no other test touches global document state)

Irrelevant to this PR but ... if I'm understanding you correctly: I think this is not true. document.createElement does not modify the actual global document. It's simply a function create elements and does not create it within the document (you need to attach/append items you've created yourself).

@dbjorge
Copy link
Copy Markdown
Contributor Author

dbjorge commented Aug 14, 2019

@waabid I agree/understand that document.createElement doesn't add the element as a child element of the global document implicitly, but it does still maintain a link to the global document (via ownerDocument) which some of our product code does traverse. I don't think any of our current tests are broken because of this behavior, but I think on principle it makes more sense to try to give the tests isolated test data from one another where there is an easy mechanism to do so

@waabid
Copy link
Copy Markdown
Member

waabid commented Aug 14, 2019

@waabid I agree/understand that document.createElement doesn't add the element as a child element of the global document implicitly, but it does still maintain a link to the global document (via ownerDocument) which some of our product code does traverse. I don't think any of our current tests are broken because of this behavior, but I think on principle it makes more sense to try to give the tests isolated test data from one another where there is an easy mechanism to do so

Ooooh good point!

@dbjorge dbjorge merged commit bee2463 into microsoft:master Aug 14, 2019
@dbjorge dbjorge deleted the replace-nodeselector-with-document branch August 14, 2019 22:20
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.

3 participants