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

Move fork back into main project #362

Merged
merged 180 commits into from
Feb 7, 2022
Merged

Move fork back into main project #362

merged 180 commits into from
Feb 7, 2022

Conversation

fb55
Copy link
Collaborator

@fb55 fb55 commented Dec 12, 2021

Ivan thankfully added some contributors, and it's time to move my fork back into the main project. Not sure yet about the best approach, open to suggestions.

Things left to do:


Changelog:

  • Ported to TypeScript
  • Switched to npm workspaces, in favour of lerna
  • Switched to Jest as the test runner
  • Switched to the entities module for decoding entities
    • A version of parse5's decoding logic was adapted for entities. Adopting this dependency allows us to share the maintenance with the entities & htmlparser2 modules.
  • Moved the docs back to TSDoc comments
  • Switched to the state machine pattern of htmlparser2 for tokenizer
  • No more mixins: Merged location & error mixins into the main classes
  • Introduced tag IDs, to avoid branching over large numbers of strings.
  • Introduced Maps and Sets where appropriate
  • Switched the order of the formatted elements list
  • Introduced array helpers in a lot of places
  • Updated tests to no longer build objects of tests (used describe/it constructs instead)

@wooorm
Copy link
Collaborator

wooorm commented Dec 19, 2021

It’s a bit hard to review (I also get that it would be too much work to do it in many separate PRs). Anything you want me to look at?

@fb55 fb55 marked this pull request as draft December 27, 2021 14:08
@fb55
Copy link
Collaborator Author

fb55 commented Dec 27, 2021

It’s a bit hard to review (I also get that it would be too much work to do it in many separate PRs). Anything you want me to look at?

Not for now. I realised that I should have made this a draft until the TODOs are resolved — just made it so.

Once it is ready: My ask would be that you (1) do a check of the general setup for anything that stands out to you (especially with the ESM setup — I don't feel confident here), and (2) try to use it with your dependent modules. I found some breakages when updating Cheerio and JSDOM to use my version, and more checks are very welcome.

@wooorm
Copy link
Collaborator

wooorm commented Dec 27, 2021

sounds good!

fb55 and others added 24 commits January 7, 2022 00:36
`generate-parsing-tests` is now polymorphic.
Bumps [eslint](https://github.com/eslint/eslint) from 8.1.0 to 8.2.0.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md)
- [Commits](eslint/eslint@v8.1.0...v8.2.0)

---
updated-dependencies:
- dependency-name: eslint
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
`Set` seems to be faster in these use-cases, and it makes the code a lot more readable.
Bumps [lint-staged](https://github.com/okonet/lint-staged) from 12.1.7 to 12.2.0.
- [Release notes](https://github.com/okonet/lint-staged/releases)
- [Commits](lint-staged/lint-staged@v12.1.7...v12.2.0)

---
updated-dependencies:
- dependency-name: lint-staged
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [lint-staged](https://github.com/okonet/lint-staged) from 12.2.0 to 12.2.1.
- [Release notes](https://github.com/okonet/lint-staged/releases)
- [Commits](lint-staged/lint-staged@v12.2.0...v12.2.1)

---
updated-dependencies:
- dependency-name: lint-staged
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [lint-staged](https://github.com/okonet/lint-staged) from 12.2.1 to 12.2.2.
- [Release notes](https://github.com/okonet/lint-staged/releases)
- [Commits](lint-staged/lint-staged@v12.2.1...v12.2.2)

---
updated-dependencies:
- dependency-name: lint-staged
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [typescript](https://github.com/Microsoft/TypeScript) from 4.5.4 to 4.5.5.
- [Release notes](https://github.com/Microsoft/TypeScript/releases)
- [Commits](microsoft/TypeScript@v4.5.4...v4.5.5)

---
updated-dependencies:
- dependency-name: typescript
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [lint-staged](https://github.com/okonet/lint-staged) from 12.2.2 to 12.3.1.
- [Release notes](https://github.com/okonet/lint-staged/releases)
- [Commits](lint-staged/lint-staged@v12.2.2...v12.3.1)

---
updated-dependencies:
- dependency-name: lint-staged
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.10.0 to 5.10.1.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.10.1/packages/eslint-plugin)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/eslint-plugin"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#63)

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.10.0 to 5.10.1.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.10.1/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@fb55
Copy link
Collaborator Author

fb55 commented Jan 25, 2022

Thanks for your continued work. Glad that npm workspaces are working. I looked through everything except for TS code.

Thanks for doing so, and sorry for the delay to respond!

  • The PR title and changelog presumably are not up to date?

Added two points, but otherwise everything should be covered.

  • Docs are broken (they point to master here, which still includes everything). I’m still 👎 on removing all docs as it prevents us from cutting a release. I do find it acceptable if they will be added back in a different PR soon. As a separate point, which I believe you agree with (Move fork back into main project #362 (comment)), I want to stress that I don‘t think autogenerated docs are enough.

The previous docs were generated using typedoc-plugin-markdown, based on the .tsd file that used to be a part of this repo. They are moved back to TSDoc in this PR.

I agree that docs are something that should be worked on. Let's discuss this in a new issue once this PR is merged.

  • I’m still in favor of loosening semver ranges (Move fork back into main project #362 (comment)). The argument you used for updating versions all the time was “helpful PRs”, to which I can say that, I use this style in 100s of projects for and have received very few of such useless PRs. I’m not going to block on this though, but it’s a strong preference.

Sorry for not being clearer here. My main reason is that all versions in range have to be supported. By keeping versions at the latest, any future breakages are the upstream's responsibility.

I still have the confusion around security updates for css-select in mind as well, which is the point I made earlier.

  • The major change, for dependents, seems to be mixins. I don‘t think the new system, or how to migrate, is documented?

Removed mixins shouldn't require any changes for dependents. Classes no longer override their methods at runtime based on options. If anything, these changes should make monkey-patching more reliable.

It might be too much work, but when possible, I’d recommend pulling a few small things out of this PR when possible (e.g., “Switched the order of the formatted elements list”)

Finding good delineation points is difficult, and I would like to go ahead as things are right now. For users reading the changelog, I do think that the changes here make sense: changes don't change any of the externally visible behaviour, only @types/parse5 will have to be removed. I have started opening separate PRs for user-facing features and fixes.

Regarding _ Switched the order of the formatted elements list_: I swapped push/pop for shift/unshift in a few places where mostly the last 1 or 2 elements were used. Now, these will be at indices 0 and 1, which leads to arguably more readable code. This should have been part of a separate PR, but as changes are split over several commits, reverting now is difficult 😒

Future work:

  • Switch from jest to something else, use C8
  • Improve docs
  • remove parse5-fork in the parse5 org
  • point to this repo in the parse5 org

All of this makes a lot of sense to me.

@fb55
Copy link
Collaborator Author

fb55 commented Jan 25, 2022

@43081j Have you had a chance to look further into this?

@wooorm
Copy link
Collaborator

wooorm commented Jan 26, 2022

👋

My main reason is that all versions in range have to be supported.

That’s how semver (ranges) are supposed to work though.
The trust-based system doesn’t always work (a dep shipping a breaking change in a patch/minor), but no unlocked semver range prevents it.
It is infrequent that I get an issue where someone doesn’t understand npm update (estimated one or two in 2 recent years across tons of repos).

I still have the confusion around security updates for css-select in mind as well, which is the point I made earlier.

Reading the 5 issues you linked, the problem with css-select and css-what seems to be either a pinned dependency or everyone using v4 of css-select instead of the updated v5? I don’t see how the method I propose affects this.

I don’t think it’s worth optimizing for, when a sec vuln happens, 5 issues of people that don’t understand npm. Those folks will find issue trackers anyway for whatever reasons.
I’d rather optimize for a beautiful and readable commit log (I find it hard to read the css-select commits because it’s all dependabot)

Removed mixins shouldn't require any changes for dependents.

👍

changes don't change any of the externally visible behaviour, only @types/parse5 will have to be removed.

It’s hard for me to gauge whether that was the case. So, glad to hear.

Bumps [lint-staged](https://github.com/okonet/lint-staged) from 12.3.1 to 12.3.2.
- [Release notes](https://github.com/okonet/lint-staged/releases)
- [Commits](lint-staged/lint-staged@v12.3.1...v12.3.2)

---
updated-dependencies:
- dependency-name: lint-staged
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@43081j
Copy link
Collaborator

43081j commented Jan 27, 2022

@fb55 really sorry things got busy so i haven't had much chance the last few days to be on github.

i'll try get through the rest of it by sunday

dependabot bot and others added 6 commits January 31, 2022 01:38
Bumps [eslint](https://github.com/eslint/eslint) from 8.7.0 to 8.8.0.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md)
- [Commits](eslint/eslint@v8.7.0...v8.8.0)

---
updated-dependencies:
- dependency-name: eslint
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.10.1 to 5.10.2.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.10.2/packages/eslint-plugin)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/eslint-plugin"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [lint-staged](https://github.com/okonet/lint-staged) from 12.3.2 to 12.3.3.
- [Release notes](https://github.com/okonet/lint-staged/releases)
- [Commits](lint-staged/lint-staged@v12.3.2...v12.3.3)

---
updated-dependencies:
- dependency-name: lint-staged
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#67)

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.10.1 to 5.10.2.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.10.2/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Makes sure we export the proper files
dependabot bot and others added 3 commits February 7, 2022 01:43
Bumps [jest](https://github.com/facebook/jest) from 27.4.7 to 27.5.0.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/main/CHANGELOG.md)
- [Commits](jestjs/jest@v27.4.7...v27.5.0)

---
updated-dependencies:
- dependency-name: jest
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@fb55 fb55 enabled auto-merge (squash) February 7, 2022 13:59
@fb55 fb55 merged commit d938786 into inikulin:master Feb 7, 2022
@fb55
Copy link
Collaborator Author

fb55 commented Feb 7, 2022

Really happy to finally wrap this up. Thanks you so much for the reviews @wooorm and @43081j!

@fb55 fb55 mentioned this pull request Mar 24, 2022
jmbpwtw pushed a commit to jmbpwtw/parse5 that referenced this pull request Feb 16, 2023
- Ported to TypeScript
- Switched to npm workspaces, in favour of `lerna`
- Switched to Jest as the test runner
- Switched to the `entities` module for decoding entities
  - A version of parse5's decoding logic was adapted for `entities`. Adopting this dependency allows us to share the maintenance with the `entities` & `htmlparser2` modules.
- Moved the docs back to TSDoc comments
- Switched to the state machine pattern of `htmlparser2` for tokenizer
- No more mixins: Merged location & error mixins into the main classes
- Introduced tag IDs, to avoid branching over large numbers of strings.
- Introduced Maps and Sets where appropriate
- Switched the order of the formatted elements list
- Introduced array helpers in a lot of places
- Updated tests to no longer build objects of tests (used `describe`/`it` constructs instead)

Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: 43081j <43081j@users.noreply.github.com>
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.

Typescript parse5 with WTF typings PR: 7.0 - ESM - Only version
3 participants