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

[WIP] Refactored one-loader to allow more tags #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Zauberfisch
Copy link

@Zauberfisch Zauberfisch commented Sep 7, 2020

After the discussions in #44 I have decided to look into it, and ended up refactoring the whole loader.
I did that with 3 goals:

  1. make the code easier to understand
    Therefore, I've split loader.js into 2 new sub-loaders to make it more obvious what is happening
    I've also renamed some variables and added a few comments
  2. add a <template> tag
    turned out to be a very simple 1 line change
  3. allow multiple tags (eg 2 <script> tags)
    Tags now don't really matter much anymore. The only rule now is to have at least 1 script tag which will be the primary javascript (or typescript, coffee, ...) part (either the tag with id="component" or the first script tag). All other parts will be loaded into that via require() regardless of tag.
    This now means that you could in theory even have <script type="text/css">body { background: red; color: blue; }</script> and it would load correctly via the css-loader & style-loader because only the type attribute is used to decide what to do. It's silly, but it would work. Primary benefit of the tags now is just the different syntax highlighting in editors.

Because it made sense in my refactoring, I also changed the return value of the parser, and thereby fixed #52

EDIT: <template> is actually not working as intended at the moment. We need to turn the parsed tree back into a string in order to work.

This PR is not ready to be merged, but rather a request for review and comments

TODOs:

  • update tests
  • add more documentation
  • fix <template>: turn tree object into string
  • verify that this actually satisfies the usecase of "jest" unit tests

Tags can now occur multiple times
Added template tag, intended for html
Improved code readability
@Zauberfisch
Copy link
Author

Turning <template> into a string is easy enough using posthtml-renderer to turn it back into html.
But this will mess with custom content like template languages (eg handlebars, twig, ...)

So we might have to swap out the parser for something that exposes innerHTML of the node without modifying it.

@Zauberfisch
Copy link
Author

Zauberfisch commented Sep 7, 2020

Not sure how to handle the html parsing and maintaining the content as raw string.
The underlying htmlparser2 that is used by the current parser is not able to handle non-html tags (eg <% if $foo %>).
I also looked into parse5 and node-html-parser.

style script template
posthtml-renderer working working parses into tree, no way to access raw html, fails to parse custom tags
htmlparser2 working working parses into tree, no way to access raw html, fails to parse custom tags
parse5 working working parses into tree, no way to access raw html, I did not test custom tags
node-html-parser not working not working working

@Zauberfisch
Copy link
Author

In some preliminary testing, cheerio seems to be doing exactly what it should. It's using parse5 in the background, so in theory we might also get it done with just parse5, but not sure the overhead is worth it.

Of course this raises the question now: are we willing to accept a large dependency such as cheerio?
I'd like to make the case that yes, it's worth doing. The parse.js file is very small and easy to understand now and cheerio seems to be a pretty well established project with big supporters.
But it is more overhead than the old parser. Though given that it's only running at build time, I think that's fine.

@MunGell
Copy link
Collaborator

MunGell commented Sep 7, 2020

Hi @Zauberfisch

Thanks for your work! I will need a little bit of time to review this PR, but looks very promising!

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.

Rewrite to remove lodash.get and lodash.set
2 participants