-
Notifications
You must be signed in to change notification settings - Fork 27
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
Support Custom Views local development #3175
Conversation
🦋 Changeset detectedLatest commit: c9b9128 The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 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 |
Deploy preview for merchant-center-application-kit ready! ✅ Preview Built with commit c9b9128. |
@@ -92,7 +93,7 @@ function CustomViewLoader(props: TCustomViewLoaderProps) { | |||
context: { | |||
dataLocale: appContext.dataLocale, | |||
customViewConfig: props.customView, | |||
hostUrl: window.location.href, | |||
hostUrl: props.hostUrl || window.location.href, |
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 allows for mocking the URL in the local custom-view development template
@@ -0,0 +1,206 @@ | |||
{ |
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.
Custom Views have a slightly different configuration requirements than Custom Apps, so that's why we renamed the previous schema and created this one.
"build:schema": "json2ts schema.json src/schema.ts --style.singleQuote --bannerComment '/* eslint-disable prettier/prettier */\n// This file was automatically generated by json-schema-to-typescript.\n// DO NOT MODIFY IT BY HAND. Instead, modify the source schema.json file.'" | ||
"build:customApplicationSchema": "json2ts custom-application.schema.json src/custom-application.schema.ts --style.singleQuote --bannerComment '/* eslint-disable prettier/prettier */\n// This file was automatically generated by json-schema-to-typescript.\n// DO NOT MODIFY IT BY HAND. Instead, modify the source custom-application.schema.json file and run the build:schemas npm script.'", | ||
"build:customViewSchema": "json2ts custom-view.schema.json src/custom-view.schema.ts --style.singleQuote --bannerComment '/* eslint-disable prettier/prettier */\n// This file was automatically generated by json-schema-to-typescript.\n// DO NOT MODIFY IT BY HAND. Instead, modify the source custom-view.schema.json file and run the build:schemas npm script.'", | ||
"build:schemas": "pnpm build:customApplicationSchema && pnpm build:customViewSchema" |
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.
We now have to manage two schemas for configuration files (Custom Applications and Custom Views).
const customApplicationExplorer = getExplorer('custom-application-config'); | ||
const customViewExplorer = getExplorer('custom-view-config'); |
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.
We need to be able to load config files for both Custom Applications and Custom Views
@@ -26,7 +26,10 @@ beforeEach(() => { | |||
|
|||
describe('processing a simple config', () => { | |||
beforeEach(() => { | |||
loadConfig.mockReturnValue(fixtureConfigSimple); | |||
loadConfig.mockReturnValue({ |
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 function now returns not only the configuration but also the filename, so we need to update all these mocks.
@@ -52,6 +52,14 @@ const AuthenticationRoutes = (props: TAuthenticatedProps) => ( | |||
applicationMessages={props.applicationMessages} | |||
/> | |||
</SuspendedRoute> | |||
<SuspendedRoute |
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.
We use this new route in Custom Views local auth flow.
children: ReactNode; | ||
}; | ||
|
||
const CustomViewDevHost = (props: TCustomViewDevHost) => { |
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.
@@ -1,4 +1,5 @@ | |||
const util = require('util'); | |||
const { MessageChannel } = require('worker_threads'); |
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 is needed to emulate how we create a private messages channel between host and Custom View in the test environment.
patches/jsdom@20.0.3.patch
Outdated
|
||
module.exports = function (globalObject) { | ||
- return function (message, targetOrigin) { | ||
+ return function (message, targetOrigin, ports) { |
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.
We use the third parameter of postMessage
in our code to create the private channel between the host application and the Custom View but jsdom
does not support it yet:
- Here is where they have a TODO about it
- There's this issue asking for its support
- Here's a PR trying to implement this missing functionality (already 4 months old)
Since our case is very basic, I implemented this simplistic patch so it works in our tests.
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.
Not needed anymore as we are now mocking this part in a different way (mock the providers instead of relying on the communication to happen in the tests environment).
Just leave the comment here for reference purposes.
mc-scripts
cli to support Custom Views@@ -0,0 +1,107 @@ | |||
import { type ReactNode, useEffect, useRef } from 'react'; |
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 new file contains helpers specific for Custom Views tests so we don't have to include them in the template so we keep control over them.
{intl.formatMessage(messages.subtitle, { | ||
firstName: customViewContext.user?.firstName, | ||
lastName: customViewContext.user?.lastName, | ||
})} |
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 is just to showcase how to access information from the context.
if (process.env.NODE_ENV === 'development') { | ||
return ( | ||
<CustomViewDevHost | ||
environment={environment} | ||
applicationMessages={loadMessages} | ||
> | ||
<CustomViewMain | ||
customViewId={environment.customViewId} | ||
messages={loadMessages} | ||
/> | ||
</CustomViewDevHost> | ||
); | ||
} |
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.
Locally we use the new component to emulate a MC application where we can trigger the Custom View from.
import { graphql } from 'msw'; | ||
import { mapResourceAccessToAppliedPermissions } from '@commercetools-frontend/application-shell/test-utils'; | ||
|
||
export const getDefaultCustomViewServerMocks = () => [ |
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 would like to move this to the new Custom views test utils file (in the application-shell
package) but that would imply to include msw
as a dependency of that package and we don't want that (would be bloating the bundle size).
(applicationContext) => | ||
projectPermissions?.dataFences ?? applicationContext.dataFences | ||
); | ||
const { resolvedPermissions, resolvedActionRights, resolvedDataFences } = |
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.
Up until now we were reading the permissions from the application context but this is not enough when using this hook from a Custom View, since we won't have them over there but in the custom view context so I decided to create a private utility function to try to fetch permissions from both so we don't need to change the signature of this hook or create a new one.
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'm a bit confused. Isn't the ApplicationContext
also used in Custom Views? The permissions are usually exposed from the FetchProject
data and mapped into the application context.
I'm also wondering if we really need a separate context for Custom Views but (re)use the Application Context and inject some customView
specific data.
Let's have a chat about this maybe.
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.
Yes, that change wasn't actually needed. I implemented it at some point where things were not working but I think I then solved it the right way.
I rolled back that change in this commit: f4caaf5
Regarding unifying contexts, I still believe is better for organization to have different ones serving different purposes.
Also, the new one would not change at runtime because it's for improve context data to be easily available throughout the Custom View code and it will be populated with the (static) data received from the host application.
return ( | ||
<ApplicationContextProvider | ||
user={user} | ||
project={project} | ||
projectDataLocale={props.dataLocale} | ||
environment={props.environment} | ||
> | ||
{props.children} | ||
<CustomViewWithPermissionCheck |
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.
We make sure the user has, at least, view permission for the current custom view.
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.
Awesome work so far, definitely lots of places that depend on each other. Hopefully that helps you to better understand the setup 💪
I went through the changes and I ended up having several questions and clarification points.
Let's have a sync to discuss them, thanks 🤗
@@ -4,4 +4,4 @@ set -e | |||
|
|||
# Copy the custom application config `schema.json` to the website static assets. | |||
echo "Copying JSON schema for Custom Applications" | |||
cp ../packages/application-config/schema.json static/ | |||
cp ../packages/application-config/custom-application.schema.json static/ |
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.
schema.json
, at least for backwards compatibility?
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.
Ok, I guess I didn't realized this is how we host the schema publicly, right?
Perhaps we can have this kind of configuration instead:
cp ../packages/application-config/custom-application.schema.json static/schema.json
cp ../packages/application-config/custom-application.schema.json static/
cp ../packages/application-config/custom-view.schema.json static/
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.
Nit: wondering if it would make sense to reference parts of this schema from the Custom Application schema (I think using the $ref
keyword: https://json-schema.org/understanding-json-schema/structuring.html#ref), just to avoid duplication.
if (configFileName.includes('custom-application-config')) { | ||
return ConfigType.CUSTOM_APPLICATION; | ||
} else if (configFileName.includes('custom-view-config')) { | ||
return ConfigType.CUSTOM_VIEW; | ||
} else { | ||
throw new Error(`Invalid config filename: ${configFileName}`); | ||
} |
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.
Nit: the config filename cannot be invalid, as otherwise cosmiconfig should already throw.
Maybe here we can simply check for the custom-view-config
file name and fall back to a Custom Application.
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.
Thanks.
Update here: 698d360
permissions: getPermissions(customViewConfig), | ||
type: customViewConfig.type, | ||
typeSettings: customViewConfig.typeSettings, | ||
hostUrl: customViewConfig.env.development.hostUrl, |
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.
Does this need to be exposed here, since it's only defined in development?
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.
Yes this is used only in development so we can load the emulated host URL from the configuration file (you can see it in the line of the entry point file).
Maybe there is another way of doing it so I'm open for suggestions.
const context = useContext(CustomViewContext); | ||
if (!context) { | ||
type TMergedContext = TCustomViewContext & TApplicationContext<{}>; | ||
export function useCustomViewContext(): TMergedContext { |
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 you add a note to implement the selector with function overloading?
const customApplicationContext = useApplicationContext(); | ||
const customViewContext = useCustomViewContext(); |
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.
If a context is not defined, does it throw an error if you try to access it?
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.
Currently the context always have an empty config by default so it won't throw an error.
(applicationContext) => | ||
projectPermissions?.dataFences ?? applicationContext.dataFences | ||
); | ||
const { resolvedPermissions, resolvedActionRights, resolvedDataFences } = |
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'm a bit confused. Isn't the ApplicationContext
also used in Custom Views? The permissions are usually exposed from the FetchProject
data and mapped into the application context.
I'm also wondering if we really need a separate context for Custom Views but (re)use the Application Context and inject some customView
specific data.
Let's have a chat about this maybe.
const defaultCustomViewConfig: TCustomView = { | ||
createdAt: '', | ||
defaultLabel: '', | ||
id: Date.now().toString(), | ||
installedBy: [], | ||
labelAllLocales: {}, | ||
locators: [], | ||
owner: { | ||
createdAt: '', | ||
id: '', | ||
organizationId: '', | ||
updatedAt: '', | ||
}, | ||
ownerId: '', | ||
permissions: [], | ||
status: TCustomViewStatus.Draft, | ||
type: TCustomViewType.CustomPanel, | ||
typeSettings: { | ||
size: TCustomViewSize.Small, | ||
}, | ||
updatedAt: '', | ||
url: '', | ||
}; |
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.
Does this need to be hardcoded here? Isn't this parsed and loaded from the config file? 🤔
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.
We don't have all the information in the config file.
This will come from the host application in runtime but I didn't want to have a null value in the context for starters.
// Use function overloading to declare two possible signatures with two | ||
// distict return types, based on the selector function argument. | ||
function useCustomViewContextHook(): TCustomViewContext; | ||
function useCustomViewContextHook<SelectedContext>( | ||
selector: (context: TCustomViewContext) => SelectedContext | ||
): SelectedContext; | ||
|
||
// Then implement the function. Typescript will pick the appropriate signature | ||
// based on the function arguments. | ||
function useCustomViewContextHook<SelectedContext>( | ||
selector?: (context: TCustomViewContext) => SelectedContext | ||
) { | ||
const context = useContext(Context); | ||
// Because of the way the CustomViewShell configures the Context.Provider, | ||
// we ensure that, when we read from the context, we always get actual | ||
// context object and not the initial value. | ||
// Therefore, we can safely cast the value to be out `TCustomViewContext` type. | ||
const customViewContext = context as TCustomViewContext; | ||
return selector ? selector(customViewContext) : customViewContext; | ||
} | ||
|
||
// This is a workaround to trick babel/rollup to correctly export the function. | ||
// Most likely the problem arises with the use of overloading. | ||
// See related issue: https://github.com/babel/babel/issues/8361 | ||
const useCustomViewContext = useCustomViewContextHook; |
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.
Same as for useApplicationContext
, we provide the ability to pass a selector function.
const iFrameUrl = [ | ||
window.location.origin, | ||
'custom-views', | ||
props.customView.id, | ||
'projects', | ||
projectKey, | ||
].join('/'); |
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 would be the new route in the MC Proxy to serve the Custom Views
const permissionKeys = entryPointUriPathToPermissionKeys(props.customViewId); | ||
|
||
// Require View permission to render the application. | ||
const canView = useIsAuthorized({ | ||
demandedPermissions: [permissionKeys.View], | ||
}); |
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.
Currently this always yields false
, until we adjust the applied permissions logic in the MC API to expose the computed permission keys for Custom Views (for local development).
if (process.env.NODE_ENV === 'development' && !props.disableDevHost) { | ||
return ( | ||
<Suspense fallback={<ApplicationLoader />}> | ||
<CustomViewDevHost applicationMessages={props.applicationMessages}> | ||
<CustomViewShell applicationMessages={props.applicationMessages}> | ||
{props.children} | ||
</CustomViewShell> | ||
</CustomViewDevHost> | ||
</Suspense> | ||
); | ||
} |
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.
For local development we're wrapping the <CustomViewShell>
with <CustomViewDevHost>
window.app.applicationIdentifier = `__local:${CUSTOM_VIEW_HOST_ENTRY_POINT_URI_PATH}`; | ||
window.app.customViewId = CUSTOM_VIEW_ID; | ||
window.app.__DEVELOPMENT__.customViewConfig = DEMO_CUSTOM_VIEW; | ||
window.app.__DEVELOPMENT__.customViewHostUrl = window.location.href; |
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.
For now it's fine but it's not very maintainable like this.
@@ -78,7 +88,7 @@ function DemoCustomView() { | |||
} | |||
|
|||
return ( | |||
<CustomViewShell customViewId={CUSTOM_VIEW_ID} messages={{}}> | |||
<CustomViewShell disableDevHost applicationMessages={{}}> |
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 we are rendering the <CustomViewDevHost>
from within the <CustomViewShell>
, we need to explicitly opt out of it even in development, as the setup we have in playground does not need that component.
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.
Alright, there are some follow ups to look into but overall it's good to go from my side! 🚀
Amazing job Carlos for the core and demo setups. Looking forward to see this in action in the Merchant Center 🙌
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 looks great! Thanks @CarlosCortizasCT and @emmenko.
I left some minor suggestions but, generally, it's solid work.
🚀
env: { | ||
development: { | ||
// TODO: This should be populated in the template installation process | ||
initialProjectKey: 'almond-40', |
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.
NIT
initialProjectKey: 'almond-40', | |
initialProjectKey: '${env:CTP_INITIAL_PROJECT_KEY}' |
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.
Thanks 8700882
<CustomViewShell applicationMessages={loadMessages}> | ||
<AsyncApplicationRoutes /> | ||
</CustomViewShell> |
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.
💯
.changeset/nervous-bags-clap.md
Outdated
'@commercetools-frontend/application-shell': patch | ||
--- | ||
|
||
Move component to application-shell package |
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.
NIT: It will be nice to know the component(s) that were moved.
Move component to application-shell package | |
Move CustomViewLoader to application-shell package |
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.
Thanks 0f94a4a
: `/${entryPointUriPath}`; | ||
const hostUrl = new URL( | ||
hostUriPath || defaultHostUriPath, | ||
'http://localhost:3001' |
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.
'http://localhost:3001' | |
developmentAppUrl |
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.
Good catch! 88ef166
const entryPointUriPath = isCustomViewData(configurationData) | ||
? // When the application acts as the host for Custom Views, there is no real | ||
// entry point to be used, therefore we use a special identifier. | ||
CUSTOM_VIEW_HOST_ENTRY_POINT_URI_PATH | ||
: configurationData.entryPointUriPath; |
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.
That is a very good idea 🙌🏽
import { | ||
TCustomViewSize, | ||
TCustomViewType, | ||
} from '@commercetools-frontend/application-shell'; |
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.
import { | |
TCustomViewSize, | |
TCustomViewType, | |
} from '@commercetools-frontend/application-shell'; | |
import type { | |
TCustomViewSize, | |
TCustomViewType, | |
} from '@commercetools-frontend/application-shell'; |
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.
These are enums...actually let me remove these exports, we don't really need them. 4894226
Summary
This PR focuses on creating a new starter template to bootstrap a Custom View and to allow running it locally.
Description
Most of the change files are actually the new files for the new template, which was created based on the Custom Applications typescript starter template.
The main two differences are:
custom-view-config
)ApplicationShell
component in itsentry-point
but new components tailored to run the Custom View locally:CustomViewShell
: new main wrapper for Custom ViewsCustomViewDevHost
: new component to emulate a local host application where we can run the Custom View fromOther relevant changes are:
application-config
package instead of creating a new one for Custom Views.CustomDevHost
.