Skip to content

Conversation

@absidue
Copy link
Member

@absidue absidue commented Oct 25, 2022

Use native DomParser instead of opml-to-json dependency

Pull Request Type

  • Feature Implementation

Description

This pull request replaces the opml-to-json dependency with the native DomParser. I did a similar thing for parsing the RSS feeds, which went down well, so I thought I would do it for the OPML import too.

Screenshots

before:
before

after:
after

Testing

  1. yarn dev
  2. Download the 3 test files (Just a list of all the LTT channels)
  3. Before importing each of the the files, click "Remove All Subscriptions / Profiles" in the privacy settings.
  4. Import the subscription files, the broken one is intended to fail, this is to test that the error handling works.

opml-testfiles.zip

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.17.1

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 25, 2022 19:53
@github-actions github-actions bot added PR: dependencies Pull requests that update a dependency file PR: waiting for review For PRs that are complete, tested, and ready for review labels Oct 25, 2022
PikachuEXE
PikachuEXE previously approved these changes Oct 26, 2022
Copy link
Member

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Tested locally

@ChunkyProgrammer
Copy link
Member

importing is not working with my subscription list as one of the channels that I am subscribed to has a & in its name (the export needs to escape special characters now, i think).

When using the library, it just ignored this error and was still able to parse it.

channelInfo = await this.getChannelInfoLocal(channelId)
}
feedData.forEach(async (channel) => {
const channelId = channel.getAttribute('xmlUrl').replace('https://www.youtube.com/feeds/videos.xml?channel_id=', '')
Copy link
Member

Choose a reason for hiding this comment

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

Invidious can also export the subscriptions like this: https://yewtu.be/feed/channel/{CHANNELID} (for yewtu.be as the instance) so it might be worth doing a replace for everything before 'channel/' as well to support the invidious one. This could be done in a different PR if you'd prefer

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be fixed now.

@absidue
Copy link
Member Author

absidue commented Oct 26, 2022

@ChunkyProgrammer You should now be able to import your subscriptions. XML is very strict but as HTML isn't, I've made it fallback to the HTML parser. That will allow you to import subscriptions that are invalid XML but still valid HTML.

PikachuEXE
PikachuEXE previously approved these changes Oct 27, 2022
return
}
xmlDom = htmlDom
} catch {
Copy link
Member

@ChunkyProgrammer ChunkyProgrammer Oct 27, 2022

Choose a reason for hiding this comment

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

Would you need to throw another errorNode to get to this catch block?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, the docs don't mention what happens if it fails to parse HTML https://developer.mozilla.org/en-US/docs/Web/API/DOMParser/parseFromString#error_handling

The HTML parser is too lenient, for the broken file it produced this:
broken_file

Maybe I should check the error message and only fall back to the HTML parser if the HTML entity error occurs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, only doing it for a specific error won't work, as Firefox always outputs the same error if the XML is problematic. So checking the error, wouldn't work for the web build.

Copy link
Member

Choose a reason for hiding this comment

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

Might be good to log the error with the xml and then try to do it with the html dom parser

Copy link
Member Author

Choose a reason for hiding this comment

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

It'll log the error now before trying to parse it with the HTML parser.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 14, 2022
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@absidue absidue added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 14, 2022
@FreeTubeBot FreeTubeBot merged commit 8a37692 into FreeTubeApp:development Nov 15, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 15, 2022
@absidue absidue deleted the native-domparser-opml branch November 15, 2022 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants