SalesforceCommerceCloud / pwa-kit

React-based JavaScript frontend framework to create a progressive web app (PWA) storefront for Salesforce B2C Commerce.
https://developer.salesforce.com/docs/commerce/pwa-kit-managed-runtime/guide/pwa-kit-overview.html
BSD 3-Clause "New" or "Revised" License
278 stars 129 forks source link

[FEATURE] Add the ability to load site specific or custom configurations #1639

Open vinhtrinh opened 7 months ago

vinhtrinh commented 7 months ago

Is your feature request related to a problem? Please describe.

When implementing PWA apps, we need a secure location to store and retrieve site-specific and custom configurations for third-party integrations. These configurations should not be exposed to the client-side. Examples include: site preferences, third-party service API keys, etc.

Describe the solution you'd like

The custom configurations should align seamlessly with the existing application configurations functionalities.

Additional context

Some simple examples of usage:

// file: config/my-service.js
module.exports = {
    apiKey: process.env.MY_SERVICE_API_KEY,
    apiSecret: process.env.MY_SERVICE_API_SECRET
}

// file: config/site-1/default.js
module.exports = {
    someKey: 'some value',
    anotherKey: 'another value'
}

// Gets the configurations
import {getConfig, getSiteConfig} from '@salesforce/pwa-kit-runtime/utils/ssr-config'

// default application configurations
const appConfig = getConfig()

const siteConfig = getSiteConfig()
const myServiceConfig = getConfig({configFile: 'config/my-service'})
bendvc commented 7 months ago

Hi @vinhtrinh, thanks for your interest in the PWA-Kit. I have a few comments about your suggestion and ideas what we might be able to do to provide you some assistance.

First, with regards to this code :

// file: config/my-service.js
module.exports = {
    apiKey: process.env.MY_SERVICE_API_KEY,
    apiSecret: process.env.MY_SERVICE_API_SECRET
}

I want to say that, you've high lighted something that isn't currently supported, that is placing secrets in the config. The reason being is the react rendering pipeline on the server simple serializes the config without prejudice. Meaning that any secretes you have in environment variables will most likely end up on the client (bad). This shouldn't be too hard to add support for. For example we could treat any key in the config that ends with Secret as a secret and not serialize it. But that might be a breaking change, so we'd have to think of a way that isn't breaking.

We are open to contributions being an open source project, if that is something you want to look into. I'll bring it up to the team and see if its something we can get on our roadmap.

Moving on, looking at you other suggestion about site specific configurations being build in:

const siteConfig = getSiteConfig()

Unfortunately in javascript land we do not know what the current site is unless we were to look at the window.location, but then again that would only work on the browser. Fortunately this is pretty easy to solve in your application as the config files themselves are javascript. You can simply import you site specific config into the main config file and place it under the site property. So it's not going to be something that we'll be adding support for.

// config/default.js

const site1Config = require('./site-1/default.js')

export.default = {
    sites: [
         site1: sites1Config
    ]
}

Within you react app you can use the multisite hook to get you site information:

const {site} = useMultiSite()

I hope this helped.

vinhtrinh commented 7 months ago

Hi @bendvc, thank you for the detailed response.

If we want to use naming convention to exclude secret or private configs, I would suggest that use the _ prefix as it's common convention in javascript world to define a private thing. However, this new convention might conflict with existing project config names. Thereforce, this solution only suitable for the next major release.

It would be great if we can supports both because the custom configuration has it own benefits in real projects.


About the getSiteConfig() method, it's an optional helper method. If we don't know the configuration in the config file, we can change the siteId argument to required and remove the default site fallback behavior (or even remove this helper method from the package).

The main purpose of this method is provide a standard configuration structure as a reference to avoid unexpected behavior. Site configuration (or any other custom configuration files) should be placed in a sub-directory to avoid environment name conflicts. E.g., config/sites/RefArch/my-config.js, config/organization/my-config.js

For Example: the issue with the default Retail React App setup is: if developer create a new environment in the MRT named sites, the application will not work on that environment because getConfig is now resolve sites.js as application configuration instead of the default.js. This issue caused by configuration setup, and can be fixed by create a new environment with a different name. However, we can avoid this unexpected issue by move all custom configuration to sub-directories as a best practice.

bendvc commented 6 months ago

Hey @vinhtrinh

There are a lot of good ideas so I'll try to keep the message short. When it comes to supportive private secrets we most likely won't support using a configuration file as a valid place to house secrets. The reason being is that it opens up the number of attack vectors to have that sensitive data stolen, you would have your repository and the aws environment. For that reason we are currently looking into ways to support secret values where they would be only stored as environment variables.

With regards to requesting a stricter more organized configuration schema, we can take that on board. The beauty of use having javascript support for our configurations is that you can require files from any custom structure that you choose to create.

Continuing, we do already support environment specific configurations as shown in this doc.

Selective configuration.. I'm not sure I understand, can you give me an example/use case of this feature?

With respect to the example of having an environment named "sites", probably rare, but valid. I suggest that you create a bug issue for that so we can track and look into it.