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

refactor(journey-types): removes full step from error object; move types to dedicated file #112

Merged
merged 2 commits into from
Apr 5, 2023

Conversation

cerebrl
Copy link
Contributor

@cerebrl cerebrl commented Apr 3, 2023

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 the index.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.

@cerebrl cerebrl requested a review from ryanbas21 April 3, 2023 16:28
@cerebrl cerebrl temporarily deployed to Preview April 3, 2023 16:29 — with GitHub Actions Inactive
@ryanbas21
Copy link
Contributor

Your preview environment pr-112-ryanbas21 has been deployed.

Preview environment endpoint is available here

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Comment on lines +43 to +46
"uuid": "9.0.0",
"xss": "1.0.14",
"zod": "3.21.4"
Copy link
Contributor Author

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",
Copy link
Contributor Author

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,
Copy link
Contributor Author

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.

Comment on lines -15 to -18
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>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving to types.ts.

Copy link
Contributor Author

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.

@ryanbas21 ryanbas21 temporarily deployed to Preview April 3, 2023 20:10 — with GitHub Actions Inactive
this updates typescript and some dependent versions on the older ts package
@ryanbas21 ryanbas21 temporarily deployed to Preview April 3, 2023 20:23 — with GitHub Actions Inactive
@ryanbas21 ryanbas21 temporarily deployed to Preview April 3, 2023 21:07 — with GitHub Actions Inactive
@ryanbas21 ryanbas21 temporarily deployed to Preview April 3, 2023 23:18 — with GitHub Actions Inactive
Copy link
Contributor

@ryanbas21 ryanbas21 left a comment

Choose a reason for hiding this comment

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

@cerebrl cerebrl added this pull request to the merge queue Apr 5, 2023
Merged via the queue into beta with commit b48fdd2 Apr 5, 2023
@cerebrl cerebrl deleted the refactor_journey-types branch April 5, 2023 15:36
@ryanbas21
Copy link
Contributor

🎉 This PR is included in version 1.0.0-beta.17 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ryanbas21
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants