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

Proposal for parser API #4

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

carlomorgenstern
Copy link

This is my proposal for the lib's API for now. It also comes with the corresponding functional enhancements:

  • Language support via constructor & setter
  • Custom request function support via constructor. Allows for much more customization by users, e.g. rate limiting, authorization, that doesn't need to concern the parser lib. Also allows subclasses to set headers, which is needed for mount and minion parsers (mobile user agent needed)
  • "parse" function renamed to "get", with character ID and columns params. This removes the express request object from the lib's interface. I also moved the column name logic to the server.js, IMO it should be the responsibility of the user to pass useful column names or we add a functionality for subclasses to validate these names.
  • Added possiblity for subclasses to modify the parse output before returning.

Still missing:

  • Proper per class & per method documentation. I was unsure what we want to use for that (TSDoc?) and ran out of time.
  • "Meta" or "All" profile: A parser using all currently implemented profiles and using column name logic currently in server.ts. Goal of this parser could be to more closely resemble XIVApis /character output without the user having to care about the different profiles and implementation details.

Sorry that I couldn't contribute more for now, especially the missing docs are bumming me out, but the next weeks are going to be quite busy for me. Maybe this can be a good starting point for more improvements.

Copy link
Contributor

@Supamiu Supamiu left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR and your time spent on it 🎉

I'm really happy to review such a PR, and overall the implementation is quite good, just needs some tweaks to make sure it's flexible enough 👍

Comment on lines +12 to +17
const lodestoneBaseUrls: Record<Language, string> = {
'en': 'https://na.finalfantasyxiv.com/lodestone',
'de': 'https://de.finalfantasyxiv.com/lodestone',
'ja': 'https://jp.finalfantasyxiv.com/lodestone',
'fr': 'https://fr.finalfantasyxiv.com/lodestone',
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what the language support would have to offer, since we're getting a lot of things in english so we can convert to ids that can then be used to get the name of the element in any language (including ko/zh) using the ID itself (and it's lighter for the reply, too)

Copy link
Author

Choose a reason for hiding this comment

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

I didn't see any of the "convert to ID" in the functionality of the codebase yet, sorry!
I assumed it would be more light-weight in terms of requests to the Lodestone (which is the main limiting factor for this lib AFAIK) to request data in the language needed, rather than requesting IDs and then later resolving them with a new request.
An alternative would be to resolve the IDs locally, but that would involve some kind of DB, maybe saved locally alongside the lib, which would produce a lot of complexity in code and keeping the DB up to date.
What would be your alternative approach to language support?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think Nodestone should provide language support at all, using IDs will make sure that the data can be used in any language, as it becomes the consumer's responsability to convert to string for their own display.

It also makes the response payload way lighter by not including strings.

Also, every XIVAPI entity that has a name has the name in 4 languages, having nodestone only provide one language based on the selected language would push people into making 4 requests just for localization purpose, which would be a bad thing overall.

return data;
}

public async get(characterId: string, columns: string[] = []): Promise<Record<string, unknown>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure making it get with only one parameter is the way to go for the API. While it serves the purpose for character data, having an ID as parameter makes no sense for the search endpoints, naming it characterId also makes it "character-oriented" which is not the goal of the PageParser class, since it's here to carry all the parsing logic for anyone to use.

Maybe this means that we should have an abstract class CharacterPareParser extends PageParser that provides common character logic without making PageParser too character oriented.

Copy link
Author

Choose a reason for hiding this comment

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

I was too narrow with arguments of the PageParser here, since I did not see the full scope of the project yet, sorry!
I was too focused on the character/profile pages, since that was what I needed for my use case.

As discussed on Discord already, we should probably use an options object to provide more flexibility for the subclasses.
I also agree with your idea of specializing a "character page parser" class and will update the PR accordingly.
Since the "character" pages are called "profile" in the lodestone-css-selectors, do we want to name it ProfilePageParser?

Copy link
Contributor

Choose a reason for hiding this comment

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

With the additions in #6 , I don't think a Character parser abstract class is needed anymore, since in the end, it's just one parser per request, and everything about a character is handled by the same parser, except minions and mounts but they'll work fine with the new options-based API 👍

src/core/page-parser.ts Show resolved Hide resolved
@@ -8,8 +7,7 @@ export class Achievements extends PageParser {
return achievements;
}

protected getURL(req: Request): string {
return "https://na.finalfantasyxiv.com/lodestone/character/" + req.params.characterId + "/achievement";
protected getLodestonePage(characterId: string): Promise<string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

The original purpose of passing the Request object was to give the class access to query string and params at the same time, since query string could also contain paging information for when it'll be implemented, what are your plans regarding that?

Copy link
Author

Choose a reason for hiding this comment

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

I think we should use a similar approach like the options object parameter in the get method here.

I would strongly advise against using a type from an unrelated library (Request is from express here) as users of nodestone might not actually use express in their code (e.g. for Discord bots or just random local scripts).

I don't think there is any better way than just extending the options object with everything needed for a parser (e.g. paging information) and then passing it through to the getURL/getLodestonePage function, which might sound like a lot of work. But with the current Request solution you would want to document all needed parameters of the Request object anyway, so people who don't use express can mock it without running into errors, which is basically the same amount of work. Then this would just eliminate the Request type from the nodestone lib and make the interface more explicit, since you have the expected parameters as types/generated docs rather than manually maintained docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I strongly agree with that, moving to our own Option object will greatly help with that :)

@carlomorgenstern carlomorgenstern marked this pull request as draft November 11, 2021 18:42
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.

2 participants