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

Refactor parser into a page instance variable #1226

Closed
wants to merge 1 commit into from

Conversation

ang-zeyu
Copy link
Contributor

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Other, please explain: small code refactor, originally part of #1189 ( decided to extract this out as well so less changes are observed over there )

What is the rationale for this request?
We use parser ( markbinder ) in several places in Page.js now. Passing it around as a parameter isn't really ideal (#1225), hence, let's make it an instance variable instead.

What changes did you make? (Give an overview)

  • change markbinder variable name to parser
  • extract parser as an instance variable, and tweak the logic for creating the resolveDependency promises accordingly.
  • remove includedFiles collection in resolveDependency ( we can just collect it once in page.generate() now, since there's only one parser )

Is there anything you'd like reviewers to focus on?
na

Testing instructions:
npm run test should pass

Proposed commit message: (wrap lines at 72 characters)
Refactor parser into a page instance variable

@ang-zeyu ang-zeyu requested a review from marvinchin May 23, 2020 12:37
This was referenced May 28, 2020
@ang-zeyu ang-zeyu removed the request for review from marvinchin May 28, 2020 12:10
@ang-zeyu
Copy link
Contributor Author

shelving this for now, although not as clean, realised there's some benefits to constructing a new instance for each generation, especially with regards to plugin hooks. Perhaps a better solution will soon be found.

@ang-zeyu ang-zeyu closed this May 28, 2020
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.

None yet

1 participant