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

Extract component preprocessor into a class #1309

Merged
merged 5 commits into from
Aug 2, 2020

Conversation

ang-zeyu
Copy link
Contributor

@ang-zeyu ang-zeyu commented Aug 2, 2020

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

• [x] Other, please explain:

Another part of #810 (leaving render and a few other utility methods to refactor out in Parser.js)

What changes did you make? (Give an overview)

  • make componentPreprocessor a class to avoid passing configuration parameters all throughout its methods
  • move includeFile into componentPreprocessor to consolidate functionality
  • adapt tests

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

Testing instructions:

  • npm run test passes

Proposed commit message: (wrap lines at 72 characters)
Extract componentPreprocessor into a class

The includeFile process depends on heavily on configuration parameters
being passed repeatedly throughout its functions (in
componentPreprocessor).
Let's make componentPreprocessor a class instead, passing these
configurations as instance properties.

The Page model's included sources are collected in instance properties
of Parser and then reconsolidated in page generation.
Let's create a PageSources model to hold these properties instead, and
correspondingly move them to the ComponentPreprocessor class.
This clearly defines the schema and purpose of a Page's sources.

The entry point to ComponentPreprocessor resides in the includeFile
method, which resides in Parser.
Let's move this to the ComponentPreprocessor class to benefit from the
instance properties, and also to consolidate include functionality
as well.

The includeFile process depends on heavily on configuration parameters
being passed repeatedly throughout its functions (in
componentPreprocessor).

Let's make componentPreprocessor a class instead, passing these
configurations as instance properties.
The Page model's included sources are collected in instance properties
of Parser and then reconsolidated in page generation.

Let's create a PageSources model to hold these properties instead, and
correspondingly move them to the ComponentPreprocessor class.
This clearly defines the schema and purpose of a Page's sources.
The entry point to ComponentPreprocessor resides in the includeFile
method, which resides in Parser.

Let's move this to the ComponentPreprocessor class to benefit from the
instance properties, and also to consolidate include functionality.
@ang-zeyu ang-zeyu added this to the v2.15.3 milestone Aug 2, 2020
@ang-zeyu
Copy link
Contributor Author

ang-zeyu commented Aug 2, 2020

@marvinchin just an update on the changes here: rather large diff (hence difficult to review), but mostly just syntactic indentation / whitespace diffs. 🙂

The change here mainly is a. to make componentPreprocessor a class (hence the diffs) b. "cut and paste" includeFile out of Parser.js so we can remove Parser.js entirely (as part of #810).

Will be dealing with the remaining methods in Parser.js in 2 more upcoming PRs!

@ang-zeyu ang-zeyu merged commit 81c3300 into MarkBind:master Aug 2, 2020
@ang-zeyu ang-zeyu changed the title Extract component preprocessor Extract component preprocessor into a class Aug 2, 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.

1 participant