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

refactor(parser): Remove _bootstrap method #384

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

fb55
Copy link
Collaborator

@fb55 fb55 commented Jan 18, 2022

The _bootstrap method leaves the parser in a broken-by-design state, which requires us to use non-null assertions to make TS happy. This PR removes this method, moving its arguments to the constructor.

This helps considerably with #377.

@fb55 fb55 force-pushed the no-more-boostrap branch 2 times, most recently from 94b46d0 to 4717e60 Compare January 18, 2022 14:29
@wooorm
Copy link
Collaborator

wooorm commented Jan 19, 2022

I’m a heavy user of the internals of parse5, including _bootstrap. I hope there will be alternatives?

@fb55
Copy link
Collaborator Author

fb55 commented Jan 19, 2022

The arguments of _bootstrap are now passed to the constructor. Please let me know if that works for you!

The rationale behind this change: Afaict the original intention of _bootstrap was to reuse the same parser instance multiple times. That goal isn't quite met, as all of the child classes are re-instantiated anyhow. By removing _bootstrap, we eliminate a state of the parser that is broken by design (instantiated but not bootstrapped).

@wooorm
Copy link
Collaborator

wooorm commented Feb 4, 2022

The arguments of _bootstrap are now passed to the constructor. Please let me know if that works for you!

That seems fine. I wanted to stress that I’m using the internals of parse5 to do some magic. While hacky and not pretty, I’d rather not see all internals hidden away.
This change seems unrelated, so that’s fine!

@fb55 fb55 marked this pull request as ready for review February 7, 2022 14:28
@@ -82,7 +82,7 @@ export interface ParserOptions<T extends TreeAdapterTypeMap> {
*
* @default `true`
*/
scriptingEnabled?: boolean | undefined;
scriptingEnabled?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it intentional that the value can’t be undefined, but that the field can be missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These values can still be explicitly undefined — I am honestly not sure why the | undefined was here in the first place (this was copied from the previous @types package). My suspicion would be an older TS version?

The only difference is that now Required<> works with the options.

Copy link
Collaborator

@wooorm wooorm Feb 13, 2022

Choose a reason for hiding this comment

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

I know that DefinitelyTyped was adding explicit | undefined to types, when they had ?. They‘re planning to pull them apart: DefinitelyTyped/DefinitelyTyped@b24e5f6.

My recommendation is to add | undefined if the value can be set to undefined. And to use ? when the field can be missing. And to use both, for both.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh, thanks for digging this up. Looks like the option is now called exactOptionalPropertyTypes and is explicitly opt-in (no longer enabled with strict): microsoft/TypeScript#44626

My intuition is that users enabling this flag wouldn't want to be able to pass undefined here.

packages/parse5-parser-stream/lib/index.ts Outdated Show resolved Hide resolved
@fb55 fb55 merged commit ecce4ac into inikulin:master Feb 24, 2022
@fb55 fb55 deleted the no-more-boostrap branch February 26, 2022 21:12
fb55 added a commit to parse5/parse5-fork that referenced this pull request Mar 2, 2022
jmbpwtw pushed a commit to jmbpwtw/parse5 that referenced this pull request Feb 16, 2023
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