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

Fix SelfDescribingJson type to allow optional keys in type parameter (close #1203 and #1304) #1330

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

jethron
Copy link
Contributor

@jethron jethron commented Jul 17, 2024

Record<keyof T, has the unfortunate side-effect of erasing the optional modifier for any properties in T, essentially mandating Required<T> instead of just T.

Explicitly using an index type removes this erasure while still being largely compatible with the rest of the interface expressed by Record.
An index type accepting only strings is preferable over plain {} because that allows arrays for data, which we don't support AFAIK outside of special cases like payload_data and contexts.

Should fix #1203 and #1304.

@jethron jethron force-pushed the PE-4998-sdjtype branch 2 times, most recently from 1c3417f to 851c6a7 Compare July 17, 2024 03:39
Copy link

bundlemon bot commented Jul 17, 2024

BundleMon

Files added (6)
Status Path Size Limits
libraries/browser-tracker-core/dist/index.mod
ule.js
+27.45KB 28KB / +10%
trackers/javascript-tracker/dist/sp.js
+26.47KB 25.5KB / +10%
trackers/javascript-tracker/dist/sp.lite.js
+15.58KB 16KB / +10%
trackers/browser-tracker/dist/index.umd.min.j
s
+15.45KB 16KB / +10%
libraries/tracker-core/dist/index.module.js
+13.64KB 15KB / +10%
trackers/browser-tracker/dist/index.module.js
+3.49KB 5KB / +10%

Total files change +102.07KB 0%

Final result: ❌

View report in BundleMon website ➡️


Current branch size history

Copy link
Contributor

@matus-tomlein matus-tomlein left a comment

Choose a reason for hiding this comment

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

Would like to get feedback from @igneel64 on this but LGTM!

@matus-tomlein
Copy link
Contributor

Sorry @jethron, you might need to rebase this branch as I rebased the release branch on top of master.

@matus-tomlein matus-tomlein changed the title Fix SelfDescribingJson type to allow optional keys in type parameter Fix SelfDescribingJson type to allow optional keys in type parameter O Sep 27, 2024
@matus-tomlein matus-tomlein changed the title Fix SelfDescribingJson type to allow optional keys in type parameter O Fix SelfDescribingJson type to allow optional keys in type parameter (close #1203 and #1304) Sep 27, 2024
@matus-tomlein
Copy link
Contributor

@jethron can you please merge this into the release branch? I think we can move forward with this.

@jethron jethron merged commit 100363e into snowplow:release/4.0.0 Oct 1, 2024
0 of 2 checks passed
@jethron jethron deleted the PE-4998-sdjtype branch October 1, 2024 00:35
jethron added a commit to jethron/snowplow-javascript-tracker that referenced this pull request Oct 4, 2024
jethron added a commit to jethron/snowplow-javascript-tracker that referenced this pull request Oct 4, 2024
jethron added a commit to jethron/snowplow-javascript-tracker that referenced this pull request Oct 10, 2024
Include some Snowtype-generated code as tests.
jethron added a commit to jethron/snowplow-javascript-tracker that referenced this pull request Oct 10, 2024
Include some Snowtype-generated code as tests.
jethron added a commit to jethron/snowplow-javascript-tracker that referenced this pull request Oct 10, 2024
Include some Snowtype-generated code as tests.
jethron added a commit to jethron/snowplow-javascript-tracker that referenced this pull request Oct 10, 2024
Include some Snowtype-generated code as tests.
jethron added a commit that referenced this pull request Oct 14, 2024
Include some Snowtype-generated code as tests.
matus-tomlein pushed a commit that referenced this pull request Oct 25, 2024
Include some Snowtype-generated code as tests.
@matus-tomlein matus-tomlein mentioned this pull request Oct 25, 2024
matus-tomlein pushed a commit that referenced this pull request Oct 28, 2024
Include some Snowtype-generated code as tests.
matus-tomlein pushed a commit that referenced this pull request Oct 28, 2024
Include some Snowtype-generated code as tests.
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.

2 participants