refactor(parser): Remove _bootstrap method#384
Conversation
fb55
force-pushed
the
no-more-boostrap
branch
2 times, most recently
from
94b46d0 to
4717e60
Compare
|
I’m a heavy user of the internals of parse5, including |
|
The arguments of The rationale behind this change: Afaict the original intention of |
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. |
fb55
force-pushed
the
no-more-boostrap
branch
from
4717e60 to
ffa6722
Compare
| * @default `true` | ||
| */ | ||
| scriptingEnabled?: boolean | undefined; | ||
| scriptingEnabled?: boolean; |
There was a problem hiding this comment.
Is it intentional that the value can’t be undefined, but that the field can be missing?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
2 participants
The
_bootstrapmethod 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.