-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor(journey-types): removes full step from error object; move types to dedicated file #112
Conversation
Your preview environment pr-112-ryanbas21 has been deployed. Preview environment endpoint is available here |
.svelte-kit/tsconfig.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we're still getting auto-formatting changes on these files. Can we double check that the files maintained by others aren't linted or formatted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is the death of me.
"uuid": "9.0.0", | ||
"xss": "1.0.14", | ||
"zod": "3.21.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all three of these are not maintained by official communities or companies, I'm locking them down. I've reviewed their current version, and they look good. We just need to periodically assess the latest version and manually update them.
For Zod, let's keep in eye on this PR: colinhacks/zod#2239
@@ -86,7 +86,7 @@ | |||
"autoprefixer": "^10.4.13", | |||
"babel-loader": "^9.1.2", | |||
"chromatic": "^6.17.0", | |||
"color": "^4.2.3", | |||
"color": "4.2.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locking these as well. Since they are just dev deps, we don't need to keep as close an eye on them.
@@ -322,8 +322,7 @@ export function initialize(initOptions?: StepOptions): JourneyStore { | |||
error: { | |||
code: nextStep.getCode(), | |||
message: failureMessageStr, | |||
// TODO: Should we remove the callbacks for PII info? | |||
step: prevStep?.payload, | |||
stage: prevStep?.payload?.stage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change and the one below is to tighten up what we return to the client and ensure we don't get any personal info leakage. For now, let's just provide the stage
value for error tracking purposes. If the dev needs more into, we can discuss it.
export type ConfigurationApi = ReturnType<typeof api.configuration>; | ||
export type JourneyApi = ReturnType<typeof api.journey>; | ||
export type UserInfoApi = ReturnType<typeof api.user.info>; | ||
export type UserTokensApi = ReturnType<typeof api.user.tokens>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving to types.ts
.
src/lib/widget/types.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added all the types a dev may need. We can always add more if the need arises.
…pes to dedicated file
02bf497
to
82ec92b
Compare
this updates typescript and some dependent versions on the older ts package
82ec92b
to
6eb2eec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#verbatimmodulesyntax
Worth a read. I thought it was interesting
LGTM
🎉 This PR is included in version 1.0.0-beta.17 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
We are simplifying the error object on the journey event to prevent potential PII from being exposed. Rather than sharing the full step, we are now just sharing the
stage
value. If devs need more information for analysis and tracking, we can add a more sophisticated way to share the error-triggering-step with the dev.We are also moving types to a
types.ts
file, splitting it out of theindex.svelte
file. This allows better separation and management as types grow in the project.This also locks the versions of dependencies that are not managed by organizations/companies, and we have manually reviewed the module. We will manually manage these dependencies with periodic review and updating to prevent supply-chain vulnerabilities.