-
Notifications
You must be signed in to change notification settings - Fork 334
fix: Novel fire chapter fetch issue #2099
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
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anyway to dynamically get this number of chapters per page instead of hard coding?
In case NovelFire changes it again in future, it would need a plugin update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I've got after a bit of looking around. But this requires an extra API call, so not sure if there are better solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if we want to have a fallback -
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a bad idea to make the extra request first to get the correct number of pages. The getAllChaptersForce already makes a separate request for each page. The only other solution I have in mind would be to fetch the total amount of chapters from the main novel page and then keep iterating through pages until all the chapters are fetched. Either way is one more request. I'm not great at Javascript, so not sure what the exact implementation would look like, I just wanted to at least get the plugin into a working state.
Looking a little bit further in though, it looks like the page that the parsePage function fetches has the latest chapter number under the CSS selector: "header.container > p:nth-child(5) > a:nth-child(1)". I think it would be possible to just make the fetchPage function additionally return that value on top of what it returns already so we can have the total amount of chapters without making an extra request since we would always be grabbing the first page no matter what. Once we have that getAllChaptersForce would just have to be modified so it keeps fetching the next page until allChapters.length() is equal to the most recent chapter.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might involve more changes than expected, as the API seems to be designed to do things in paralel based on the total number of pages passed along.
I am in favour of getting the fix in firstly to resolve the issue.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this might fix it for now but it should have a dynamic fix that contends to future updates and changes to the site. All in all, I am in favor of pushing this fix for now. It's feasible and will get the job done.
Also, I am a newbie at this kind of thing and would like your help in fixing it for myself temporarily because don't know when the issue will get fixed officially.
Thanks and good work, mate!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this PR mostly as a hotfix to get things working again, I agree that it needs a minor revamp to prevent this issue in the future.
You should be able to fork the repo, make the one line change to the file, and then run "npm run publish:plugins -- --all-branches" on your local copy of the repo. I couldn't get the GitHub action working on my fork, but manually running the command worked. You can then add your repo as a plugin repo in the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thank you!