one-loader icon indicating copy to clipboard operation
one-loader copied to clipboard

[WIP] Refactored one-loader to allow more tags

Open Zauberfisch opened this issue 4 years ago • 4 comments

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
  • [x] fix <template>: turn tree object into string
  • [ ] verify that this actually satisfies the usecase of "jest" unit tests

Zauberfisch avatar Sep 07 '20 00:09 Zauberfisch

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 avatar Sep 07 '20 01:09 Zauberfisch

Not sure how to handle the html parsing and maintaining the

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 avatar Sep 07 '20 02:09 Zauberfisch

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.

Zauberfisch avatar Sep 07 '20 03:09 Zauberfisch

Hi @Zauberfisch

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

MunGell avatar Sep 07 '20 20:09 MunGell