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

allow defining the element used when hydrating svelte components #221

Merged
merged 4 commits into from
Dec 1, 2021

Conversation

douglasward
Copy link

I found myself wanting to use a svelte component as a shortcode inside a table. This didn't work because the div element is used as the base element for the svelte component during the hydration preparation.

This pull request aims to allow the setting of a custom element to be used instead and if absent the default div element is used.

Example shortcode usage:

<table>
  {%svelteComponent name='TableRows' props='{}' options='{"element": "tbody"}' /%}
</table>

This worked in the project I would need it for, but before I go any further with it (including updating the tests) I wanted to check in with you to find out if this is something you would consider merging and what the prerequisites would be.

Copy link
Contributor

@nickreese nickreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Great job, makes a ton of sense. Something I've considered and even discussed with the team.

Tests to cover the functionality and if there is a way to ensure that the regex doesn't get overly greedy that would be ideal.

@@ -14,7 +14,7 @@ export default function mountComponentsInHtml({ page, html, hydrateOptions }): s
let outputHtml = html;
// sometimes svelte adds a class to our inlining.
const matches = outputHtml.matchAll(
/<div class="ejs-component[^]*?" data-ejs-component="([A-Za-z]+)" data-ejs-props="({[^]*?})" data-ejs-options="({[^]*?})"><\/div>/gim,
/<.+? class="ejs-component[^]*?" data-ejs-component="([A-Za-z]+)" data-ejs-props="({[^]*?})" data-ejs-options="({[^]*?})"><\/.+?>/gim,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only thing I'm unsure of is if there is a way we can ensure that these two tags match. My regex isn't amazing, but I'm always concerned about greedy calls.

Copy link

@vollnhals vollnhals Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be possible with a backreference. I also replaced .+? with \S+ to have a narrower match.

Suggested change
/<.+? class="ejs-component[^]*?" data-ejs-component="([A-Za-z]+)" data-ejs-props="({[^]*?})" data-ejs-options="({[^]*?})"><\/.+?>/gim,
/<(\S+) class="ejs-component[^]*?" data-ejs-component="([A-Za-z]+)" data-ejs-props="({[^]*?})" data-ejs-options="({[^]*?})"><\/\1>/gim,

The captured groups in the matches below then needs their indices changed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vollnhals, I have implement this in e74afb7

@nickreese
Copy link
Contributor

@douglasward This is good to merge once we get all of the tests working. I've also written tentative docs for this functionality.

@douglasward
Copy link
Author

@nickreese that sounds great! Sorry for the slow reply, it was one of those weeks...

I have made a start updating the tests. Lots were failing because of the new JSON.parse that we do in inlinePreprocessedSvelteComponent to merge the passed in string options with the default - it wasn't being passed valid JSON. Not sure if this is just a problem with how the tests are set up, or whether this is going to cause problems with people's projects as well.

The DATEPICKER tests are still failing and I haven't worked out why yet. Here is an example of a failing expect:

    Expected: "<div class=\"svelte-datepicker\"><div class=\"datepicker-component\" id=\"datepickerSwrzsrVDCd\"><div>DATEPICKER</div></div></div>"
    Received: "<<div class=\"datepicker-component\" id=\"datepickerSwrzsrVDCd\"><div>DATEPICKER</div></div> class=\"svelte-datepicker\"><div class=\"ejs-component\" data-ejs-component=\"Datepicker\" data-ejs-props=\"{ \"a\": \"b\" }\" data-ejs-options=\"{ \"loading\": \"lazy\", \"element\": \"div\" }\"></div></div>"

Let me know if you have any ideas or pointers.

@douglasward
Copy link
Author

@nickreese I have found and fixed the cause of the failing DATEPICKER tests... I accidentally bumped a match index that was before the index of match I had introduced, sorry.

So tests are all green. Have you had a chance to look over that changes I have made to them?

@nickreese
Copy link
Contributor

@douglasward I have. Whole family got sick with COVID. Still fighting it. Thanks for getting the tests passing. Will look at it now.

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2021

Codecov Report

Merging #221 (5dd1fae) into master (12ecd36) will increase coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
+ Coverage   82.12%   82.14%   +0.01%     
==========================================
  Files          46       46              
  Lines        1958     1960       +2     
  Branches      463      465       +2     
==========================================
+ Hits         1608     1610       +2     
  Misses        348      348              
  Partials        2        2              
Impacted Files Coverage Δ
src/partialHydration/mountComponentsInHtml.ts 84.21% <60.00%> (ø)
src/partialHydration/inlineSvelteComponent.ts 100.00% <100.00%> (ø)
src/utils/svelteComponent.ts 93.75% <100.00%> (ø)
src/utils/validations.ts 77.77% <0.00%> (ø)
src/rollup/getRollupConfig.ts 100.00% <0.00%> (ø)
src/routes/prepareRouter.ts 62.50% <0.00%> (+0.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12ecd36...5dd1fae. Read the comment docs.

@nickreese nickreese merged commit e8435c2 into Elderjs:master Dec 1, 2021
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.

4 participants