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

chore: update parse5 to v7 #3602

Merged
merged 3 commits into from
Oct 10, 2023
Merged

chore: update parse5 to v7 #3602

merged 3 commits into from
Oct 10, 2023

Conversation

divmain
Copy link
Contributor

@divmain divmain commented Jun 30, 2023

Details

This upgrades parse5 from v6 to v7. Some things worth noting:

  • I found a bug in the template-expressions implementation during the upgrade. The fixture changes you see in the diff are the result of fixing the bug. There is one edge case that remains unresolved; I will open a work-item to address it as a follow-up, as it isn't related to the parse5 upgrade.
  • The upgrade from v6 to v7 is the largest breaking change in parse5's history. They ported to TypeScript, including some refactors of tracking internal logic.
  • Because parse5 internals were untyped prior to v6, we had our own types in place to provide some guard-rails. Those types are omitted, now that parse5 internals a typed.
  • Some of the type weirdness you see in the diff is there to work around methods and fields marked as private in parse5. They're present in the compiled JavaScript, but the type system complains without these workarounds.

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

@divmain divmain requested a review from a team as a code owner June 30, 2023 07:01
@divmain divmain marked this pull request as draft June 30, 2023 07:01
@divmain divmain marked this pull request as ready for review July 11, 2023 06:11
export type Preprocessor = Omit<Parse5Preprocessor, 'pos'> & {
pos: number;
advance: () => void;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Combine this with src/parser/expression-complex/html.ts?

} else {
super.DATA_STATE(codePoint);
// @ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a tracking issue for upstream private to protected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This issue was fixed in inikulin/parse5#996. I can come back in a second PR and remove the type workarounds that are now redundant.

Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

LGTM, conditional +1 on a few things:

  1. We figure out how <meta> and <embed> parsing changed, and if possible maintain the same behavior as before to avoid causing an observable change.
  2. We wait until after CLCO for safety's sake.

# Conflicts:
#	packages/@lwc/template-compiler/package.json
@divmain divmain merged commit 1c1687e into master Oct 10, 2023
11 checks passed
@divmain divmain deleted the divmain/upgrade-parse5 branch October 10, 2023 04:55
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.

2 participants