Skip to content

Fix SelfDescribingJson type regression from #1330 - #1347

Merged
Jethro Nederhof (jethron) merged 1 commit into
snowplow:release/4.0.0from
jethron:fix-sdj-regression
Oct 14, 2024
Merged

Fix SelfDescribingJson type regression from #1330#1347
Jethro Nederhof (jethron) merged 1 commit into
snowplow:release/4.0.0from
jethron:fix-sdj-regression

Conversation

@jethron

Copy link
Copy Markdown
Contributor

Fixes a regression from the change in #1330 where in trying to prohibit arrays as SDJ data values, I accidentally mandated that types have a string index property defined. 😅

Instead, allow any object, and just explicitly bar arrays as values. This moves the type error from the type signature to the value assignment, but I don't see a better way for TypeScript to define "accepts objects but not arrays" in type parameter constraints while still having a non-inferred default type for that parameter.

@bundlemon

bundlemon Bot commented Oct 4, 2024

Copy link
Copy Markdown

BundleMon

Files added (6)
Status Path Size Limits
trackers/javascript-tracker/dist/sp.js
+24.35KB 30KB / +10%
libraries/browser-tracker-core/dist/index.mod
ule.js
+23.45KB 25KB / +10%
libraries/tracker-core/dist/index.module.js
+19.12KB 20KB / +10%
trackers/browser-tracker/dist/index.umd.min.j
s
+17.27KB 20KB / +10%
trackers/javascript-tracker/dist/sp.lite.js
+17.21KB 20KB / +10%
trackers/browser-tracker/dist/index.module.js
+3.49KB 5KB / +10%

Total files change +104.88KB 0%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history

@matus-tomlein Matus Tomlein (matus-tomlein) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Could you add a test that tracks a self-describing event (and maybe also an entity?) using a type generated by Snowtype so that we can catch any regressions in the future?

For instance you can use something like this:

interface EventSpecification {
  id: string
  name: string
  data_product_id: string
  data_product_name: string
}

@jethron
Jethro Nederhof (jethron) force-pushed the fix-sdj-regression branch 3 times, most recently from da7730a to 4a2ee3d Compare October 10, 2024 02:36
@jethron

Copy link
Copy Markdown
Contributor Author

Could you add a test that tracks a self-describing event (and maybe also an entity?) using a type generated by Snowtype so that we can catch any regressions in the future?

Great idea! That found more issues, so I ended up needing to change way more stuff than I'd hoped. :/

This is probably a good integration test idea, to generate some known data product with the latest version of snowtype to check for regressions. Needs adding a secret to the repo though so I can't do that myself.

Include some Snowtype-generated code as tests.

@matus-tomlein Matus Tomlein (matus-tomlein) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the Snowtype generated code to tests!

It does seem more complicated but as far as I can tell it seems to work!

@jethron
Jethro Nederhof (jethron) merged commit 65feb91 into snowplow:release/4.0.0 Oct 14, 2024
@jethron
Jethro Nederhof (jethron) deleted the fix-sdj-regression branch October 14, 2024 23:56
Matus Tomlein (matus-tomlein) pushed a commit that referenced this pull request Oct 25, 2024
Include some Snowtype-generated code as tests.
Matus Tomlein (matus-tomlein) pushed a commit that referenced this pull request Oct 28, 2024
Include some Snowtype-generated code as tests.
Matus Tomlein (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

Development

Successfully merging this pull request may close these issues.

2 participants