Remove expressive layouts code duplication

[ang-zeyu]

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

• [x] Other, please explain: small code refactor

What is the rationale for this request?
Merge the logic for expressive layouts in parser into includeFile

What changes did you make? (Give an overview)

• merge said logic ( the only difference was additionalVariables, which is introduced to includeFile )
• move file I/O of includeFile into Page.js instead which was the case for includeData for expressive layouts ( small change, but I think its cleaner to move the initial I/O into Page.js )

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

Testing instructions:

• npm run test

Proposed commit message: (wrap lines at 72 characters)
Remove expressive layouts code duplication

[ang-zeyu]

passing the parser (markbinder) isn't very clean, but will be dealt with in #1189 in full (making it an instance variable)

[marvinchin]

The refactor looks good! Just a couple of thoughts about lifting the I/O part up to Page.js:

• Leaving it in will reduce duplication of the fs.readFileAsync call we have to make before every includeFile call.
• It kinda makes sense semantically (to me at least) that includeFile does the reading of the file. Also, since there should be no other reason for us to use the file content outside of this function, I think not having to pass the file content to includeFile will make the call site a little cleaner.

What are your thoughts about this?

[ang-zeyu]

The refactor looks good! Just a couple of thoughts about lifting the I/O part up to Page.js:

• Leaving it in will reduce duplication of the fs.readFileAsync call we have to make before every includeFile call.
• It kinda makes sense semantically (to me at least) that includeFile does the reading of the file. Also, since there should be no other reason for us to use the file content outside of this function, I think not having to pass the file content to includeFile will make the call site a little cleaner.

What are your thoughts about this?

Hmm, my viewpoint was from a perspective of concerns - I think parser should be concerned with as little file I/O as possible, because it only 'parses' ( and also 'renders' ).
That said many other areas of parser still necessarily do file i/o ( includes / imports ), and I don't see it going away unless we implement a file cache of sorts ( I think there was an issue about this that fizzled though since the benefits were marginal ( modern ssds are too fast 🌚 ) ).

Its a really small thing though, I'm fully open to reverting it. What do you think?

[marvinchin]

That makes sense as well. I'm happy to go with this and move towards removing I/O concerns from the parser. Let's get the conflicts resolved and we can get this in!

[ang-zeyu]

That makes sense as well. I'm happy to go with this and move towards removing I/O concerns from the parser. Let's get the conflicts resolved and we can get this in!

Done!

[marvinchin]

LGTM