feat: add custom pageview property support#16
Open
dgrebb wants to merge 2 commits intoaccuser:developfrom
Open
feat: add custom pageview property support#16dgrebb wants to merge 2 commits intoaccuser:developfrom
dgrebb wants to merge 2 commits intoaccuser:developfrom
Conversation
- adds `pageview-props` script src segment
- adds various types and guards for Plausible-enforced property limits
- defends against `svelte-preprocess` [issue with shorthand `{src}` attribute](sveltejs/svelte-preprocess#604) (accuser#12)
- updates README
- updates configuration/npm scripts for prettier@3
dgrebb
commented
Jan 7, 2024
|
|
||
| $: api = `${apiHost}/api/event`; | ||
| $: src = [ | ||
| $: plausibleSrc = [ |
Collaborator
Author
There was a problem hiding this comment.
#12 identified an issue with svelte-preprocess and the shorthand {src}
dgrebb
commented
Jan 7, 2024
| data-api={api} | ||
| data-domain={domain.toString()} | ||
| defer | ||
| src={plausibleSrc} |
Collaborator
Author
There was a problem hiding this comment.
#12 identified an issue with svelte-preprocess and the shorthand {src}
dgrebb
added a commit
to dgrebb/dgrebb.com
that referenced
this pull request
Jan 7, 2024
dgrebb
added a commit
to dgrebb/dgrebb.com
that referenced
this pull request
Jan 7, 2024
dgrebb
added a commit
to dgrebb/dgrebb.com
that referenced
this pull request
Jan 9, 2024
|
@dgrebb any chance you will continue on this? |
Collaborator
Author
|
@till absolutely - did you have any feedback on he guards I added? I have a few more moths of TS under my belt and will take a gander. With Svelte 5 on the way, I may change approach as well. I'll take a look this week. |
till
reviewed
Aug 21, 2024
dgrebb
commented
Aug 29, 2024
dgrebb
commented
Aug 29, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This adds support for custom properties in pageview events. Plausible sets limits on number and length, and this includes type guards and warnings in
devmode.The history was a bit messy, so I've recreated it from current upstream
develop. Downstreamdevelopis up to date, if you want to pull that in.Details
pageview-propsscript src segmentsvelte-preprocessissue with shorthand{src}attribute (Support SvelteKit 2 #12)The guards feel a bit too much — I'm curious if there is a more efficient, concise, Typescriptier way to do this.
isCustomPropsLimit: warns if passing more than 30 propertiesisCustomPropEntryLimit: warns if a custom prop entry is more thanlimitcharacters (300 and 2000 for key/value respectively)handleEntry: warns if a property value is aDOMinterfaceAdded some documentation for the component prop:
I'd be happy to dial the guards back and use some nested ternaries instead, but I think there may be some value in
handleEntry, a helper insrc/lib/guards.ts.It checks if any DOM instances have been passed as a property value, like a
DOMTokenList,HTMLInputElement, etc.I can also add a new route and tests for coverage. Let me know what you think!