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: migrate frontend routes and google analytics factory to ts #1405

Merged
merged 10 commits into from
Apr 8, 2021

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Mar 18, 2021

Problem

Tests

  • Create a form. Open. Submit. Check that event shows up correctly on google analytics
  • Check that /frontend/environment endpoint displays the envvars
  • Check that /frontend/features endpoint displays the list of features and their activation state

@mantariksh
Copy link
Contributor

actually since you're touching the /frontend code, maybe you can convert all the frontend routes and controller to TS? it's a tiny module anyway

@tshuli tshuli force-pushed the ref/ga-factory branch 4 times, most recently from 0e0ffc8 to 0a638df Compare April 7, 2021 05:30
@tshuli tshuli changed the title refactor: migrate google analytics factory to ts refactor: migrate frontend routes and google analytics factory to ts Apr 7, 2021
@tshuli tshuli requested a review from mantariksh April 7, 2021 06:28
@tshuli tshuli marked this pull request as ready for review April 7, 2021 06:28
src/app/controllers/frontend.server.controller.ts Outdated Show resolved Hide resolved
src/app/controllers/frontend.server.controller.ts Outdated Show resolved Hide resolved
src/app/routes/frontend.server.routes.ts Outdated Show resolved Hide resolved
@@ -147,6 +148,7 @@ const loadExpressApp = async (connection: Connection) => {
routeFunction(app)
})

app.use(FrontendRouter)
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove the /frontend prefix in frontend.routes and add it here instead

src/app/routes/frontend.server.routes.ts Outdated Show resolved Hide resolved
@tshuli
Copy link
Contributor Author

tshuli commented Apr 8, 2021

@mantariksh thanks for reviewing, for re-review

@tshuli tshuli requested a review from mantariksh April 8, 2021 03:28
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

lgtm, with a final question on file placement

src/app/factories/google-analytics.factory.ts Outdated Show resolved Hide resolved
* @returns {String} Current featureManager states
*/
export const showFeaturesStates: RequestHandler<
ParamsDictionary,
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry one more thing, I realised that all these controllers still use the old method of supplying ParamsDictionary as the first generic type argument, which we don't want as per #948. could you help change all of these to unknown pls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok edited; apart from showFeaturesStates, the rest require ParamsDictionary as the first generic type argument because they call createReqMeta(req) and req is of interface Request which requires ParamsDictionary as the first generic type argument


interface IGoogleAnalyticsFactory {
addGoogleAnalyticsData: RequestHandler<
ParamsDictionary,
Copy link
Contributor

Choose a reason for hiding this comment

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

unknown here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above, ParamsDictionary is needed

@karrui karrui deleted the ref/ga-factory branch April 21, 2021 02:17
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