-
Notifications
You must be signed in to change notification settings - Fork 4
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 up form submission to be server side #2
Conversation
`hs_context=${encodeURIComponent(JSON.stringify(hs_context))}` | ||
]; | ||
|
||
for(let key in event.payload) { |
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.
Can we just make sure Prettier runs and formats the code? Otherwise we'll have huge diffs whenever someone contributes 🙏
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.
also looks like we can use const key
here?
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.
Can we just make sure Prettier runs and formats the code? Otherwise we'll have huge diffs whenever someone contributes 🙏
Sorry, I won't be able to fix all the linter errors. The vast majority were introduced before this commit.
dave@mbp hubspot % npm run lint:fix
> @managed-components/[email protected] lint:fix
> eslint --ext .ts,.js, src --fix
/Users/dave/Work/webcm/components/hubspot/src/index.test.ts
30:17 error Unexpected empty method 'return' @typescript-eslint/no-empty-function
32:22 error Unexpected empty method 'attachEvent' @typescript-eslint/no-empty-function
33:22 error Unexpected empty method 'detachEvent' @typescript-eslint/no-empty-function
37:26 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
38:21 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
80:46 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
118:44 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
122:41 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
127:40 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
131:40 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
165:21 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
/Users/dave/Work/webcm/components/hubspot/src/index.ts
3:8 error 'UAParser' is defined but never used @typescript-eslint/no-unused-vars
137:5 error 'timestamp' is never reassigned. Use 'const' instead prefer-const
138:5 error 'email' is never reassigned. Use 'const' instead prefer-const
139:5 error 'firstName' is never reassigned. Use 'const' instead prefer-const
140:5 error 'lastName' is never reassigned. Use 'const' instead prefer-const
141:5 error 'identifyEmail' is never reassigned. Use 'const' instead prefer-const
142:5 error 'identifyUserId' is never reassigned. Use 'const' instead prefer-const
143:5 error 'po' is never reassigned. Use 'const' instead prefer-const
144:8 error 'formValues' is never reassigned. Use 'const' instead prefer-const
/Users/dave/Work/webcm/components/hubspot/src/utils.ts
6:8 error Unexpected var, use let or const instead no-var
✖ 21 problems (13 errors, 8 warnings)
These occur on f3d09fd.
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've gone ahead and done the code reformat as requested in #3.
Thanks for this! I'm not sure why it was client side, seems like our pre-MC version was server side too. |
This on the hand seems useful, because forms are confirmed to work well server-side and if I'm understanding you correctly, it sounds like you tested it and it works well with the changes? |
Yes. |
Reclosing as I see server-side-2 is okay and don't want a merge conflict. =) (Will leave a comment on #3.) |
Solves #1.
Also, the old method seemed... really bad? It wasn't even submitting the form correctly for me; I tried to troubleshoot what was wrong with it, but it was just faster to ditch it and do things server-side properly to begin with.