refactor: clean up unnecessary props and types, refactor <Route> to always render as children
#2995
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 461f7ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 35 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
emmenko
commented
Mar 10, 2023
| type Props<AdditionalEnvironmentProperties extends {}> = { | ||
| environment: TProviderProps<AdditionalEnvironmentProperties>['environment']; | ||
| type TApplicationEntryPointProps = { | ||
| environment: TProviderProps<{}>['environment']; |
There was a problem hiding this comment.
This type of change is done in many places in this PR.
- Renamed the type
- Removed the type argument
AdditionalEnvironmentProperties
emmenko
commented
Mar 10, 2023
| const browserLocale = getBrowserLocale(window); | ||
| return ( | ||
| <> | ||
| <Suspense fallback={<ApplicationLoader />}> |
emmenko
commented
Mar 10, 2023
| <ReduxProvider store={internalReduxStore}> | ||
| <ApolloProvider client={apolloClient}> | ||
| <Suspense fallback={<ApplicationLoader />}> |
There was a problem hiding this comment.
...from here.
Needed for a follow up PR.
emmenko
commented
Mar 10, 2023
| <Route | ||
| exact={true} | ||
| path="/" | ||
| render={() => { |
There was a problem hiding this comment.
No more render or component prop, routes are rendered as children of <Route>
emmenko
commented
Mar 10, 2023
| @@ -191,13 +191,9 @@ export interface ApplicationWindow extends Window { | |||
| frontendHost: string; | |||
| servedByProxy: boolean; | |||
| // Optional properties. To use them, pass them to the `additionalEnv` object of the application config. | |||
| mcProxyApiUrl?: string; | |||
There was a problem hiding this comment.
It's only used internally and should not be defined as a "default" field
emmenko
commented
Mar 10, 2023
Comment on lines
-198
to
-200
| enableSignUp?: boolean; | ||
| enableLongLivedFeatureFlags?: boolean; | ||
| useFullRedirectsForLinks?: boolean; |
emmenko
commented
Mar 10, 2023
Comment on lines
+33
to
+35
| type TAdditionalEnvironmentProperties = { | ||
| mcProxyApiUrl?: string; | ||
| }; |
There was a problem hiding this comment.
We can then simply define a type to extend the app context and include the fields that we expect here to be optionally provided (via the additionalEnv config field)
emmenko
commented
Mar 10, 2023
Comment on lines
+84
to
+85
| type TAdditionalEnvironmentProperties = { | ||
| enableLongLivedFeatureFlags?: boolean; | ||
| }; |
emmenko
commented
Mar 10, 2023
Comment on lines
+120
to
+121
| const applicationContext = useApplicationContext<{ | ||
| useFullRedirectsForLinks?: boolean; | ||
| }>(); |
|
Deploy preview for merchant-center-application-kit ready! ✅ Preview Built with commit 461f7ac. |
emmenko
commented
Mar 10, 2023
| @@ -25,14 +30,11 @@ import ApplicationEntryPoint from '../application-entry-point'; | |||
| import messages from './messages'; | |||
|
|
|||
| type QueryParams = { | |||
| projectKey: string; | |||
| projectKey?: string; | |||
There was a problem hiding this comment.
The query param is actually optional.
emmenko
commented
Mar 10, 2023
| type Props = { | ||
| projectKey: string; | ||
| type TFetchProjectProps = { | ||
| projectKey?: string; |
emmenko
commented
Mar 10, 2023
| @@ -1,4 +1,4 @@ | |||
| query FetchProject($projectKey: String!) { | |||
| query FetchProject($projectKey: String) { | |||
emmenko
force-pushed
the
nm-props-cleanup
branch
from
9175c4b
to
aed5e9f
Compare
This was referenced Mar 10, 2023
kark
approved these changes
Mar 13, 2023
emmenko
force-pushed
the
nm-props-cleanup
branch
from
aed5e9f
to
9325e29
Compare
…lways render as children
emmenko
force-pushed
the
nm-props-cleanup
branch
from
9325e29
to
461f7ac
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracted from #2821
AdditionalEnvironmentPropertiesis not really used and needed by the internal components. Let's remove it.<Route>components to render as children (as recommended when upgrading to v6).