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

Upgrades htmlparser2 to new major version ^8.0.0. #573

Merged
merged 2 commits into from
Oct 31, 2022
Merged

Upgrades htmlparser2 to new major version ^8.0.0. #573

merged 2 commits into from
Oct 31, 2022

Conversation

kedarchandrayan
Copy link
Contributor

Summary

Closes #565

What are the specific steps to test this change?

All existing behavior should remain same.

What kind of change does this PR introduce?

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other - Upgrading htmlparser2 version

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

@kedarchandrayan
Copy link
Contributor Author

Hello @boutell, this is another PR as asked by you. It has only the change which was needed for the issue.

Test cases were changed to bring the behavior in sync with the new version of htmlparser2.

Please let me know your comments and changes needed.

Thanks,
Kedar Chandrayan

@boutell
Copy link
Member

boutell commented Sep 21, 2022

Because of the parser option, which is passed on directly to htmlparser2, it is a backwards compatibility break to upgrade to a new major release of htmlparser2, unless the htmlparser2 API has not changed. Can you review the changelog of htmlparser2 to figure out if we need a major version bump for sanitize-html itself and/or a few bc wrappers to allow legacy htmlparser2 options to still work?

@boutell
Copy link
Member

boutell commented Oct 24, 2022

@kedarchandrayan reminder that there is an unanswered question blocking this PR:

Because of the parser option, which is passed on directly to htmlparser2, it is a backwards compatibility break to upgrade to a new major release of htmlparser2, unless the htmlparser2 API has not changed. Can you review the changelog of htmlparser2 to figure out if we need a major version bump for sanitize-html itself and/or a few bc wrappers to allow legacy htmlparser2 options to still work?

# Conflicts:
#	CHANGELOG.md
@kedarchandrayan
Copy link
Contributor Author

Sure @boutell, I will go through the CHANGELOG of htmlparser2 for figuring out the need for a major update here. Sorry missed your last reply.

@kedarchandrayan
Copy link
Contributor Author

Hello @boutell, htmlparser2 has src/Parser.ts which has interface ParserOptions for the parser options. The properties in this interface are the same in versions 6.0.0 and 8.0.1 (latest). So there is no change in the options.

Additionally, I went through the release changes mentioned here. Parser option change is not mentioned in any release.

Conclusion: We will not need a major bump for this PR.

@boutell boutell merged commit e1034cf into apostrophecms:main Oct 31, 2022
@boutell
Copy link
Member

boutell commented Oct 31, 2022

Thanks!

@kedarchandrayan kedarchandrayan deleted the develop branch November 1, 2022 04:35
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.

use latest htmlparser2
2 participants