Contextual Configuration File Loading (rework) #421
Conversation
|
I want to open a discussion around the following topic --> Asynchronous Configuration Loading Why did this topic come up? The question about async configuration loading most recently came up as feedback from a partner developer, although its been mentioned in the past on how we'd keep the Why would be want such a thing? The answer on why we think it's valuable to have this support for async configuration loading is to solve the problem mentioned above. So we don't have to redeploy code to see effects that have happen on the backend (business manager). **What risks might this impose? ** In the real world when we talk about async configuration loading we are talking about making a fetch request to the back end in order to get the latest site, locale, currency settings before accepting requests via the dev server or lambda function. This means that there will be potentially a small price to pay to get that information from the remote server before the server is ready to accept connections, but this will only happen once during "cold boots". So in my eyes it's not a a huge issue. What can we do about this? I'm not super familiar with this part of the SDK, but at the core of our app we export a Are there any alternatives? Possibly. If there was a way to store configurations in MRT we could potentially use MRT as a source of the configuration, meaning no need to re-deploy, but we'd still have to solve not having a single source of truth. This solution would use kinda of a copy of the configuration. What effects does it have on this PR? The main purpose of this PR is to have different configurations for different environments, but if you look closely at what is in that configuration there isn't much that really benefits from this functionality besides different site/locale/currency settings (e.g. proxy information is only useful for dev servers, remotes use what it in MRT anyway). Furthermore if we where to get site information adhoc from BM there is really no reason to have any configuration differences between one production environment and another as connections to remote servers/apis are directed by the proxies for that environment. |
the sites file is just and example.. we will remove it before merging.
Documentation on this conversation has been moved here --> https://salesforce.quip.com/6JykAlnb2rCK |
bendvc
changed the title
| // AppConfig.restore *must* come before getRoutes() | ||
| AppConfig.restore(locals, window.__PRELOADED_STATE__.__STATE_MANAGEMENT_LIBRARY) | ||
| const routes = getRoutes(locals) | ||
| const routes = getRoutes(locals, window.__CONFIG__) |
There was a problem hiding this comment.
This doesn't match the signature of getRoutes https://github.com/SalesforceCommerceCloud/pwa-kit/pull/421/files#diff-bb1f55b8c21cbabdb10e9a153bef5868e2c17ff9a1a46e658bc01a59b84f3002R412
There was a problem hiding this comment.
Good call.. I think this might have been an artifact of a previous implementation. Good eye
| // Make the config available in a isomorphic scope. Preventing us from | ||
| // having to use webpack externals when bundling the `cosmiconfig` library. | ||
| setConfig(options.mobify) | ||
|
|
There was a problem hiding this comment.
The call to updatePackageMobify mutates this config object. Wouldn't you want to do updatePackageMobify before doing setConfig?
There was a problem hiding this comment.
After inspecting don't believe that function is doing any mutations.. but it's probably wise to order it in the way you suggested.
|
Found and commented on a few bits of what I assume are debugging code. Overall, looks great. Have you run the bundle analyzer and looked at the impact on bundle-sizes for front and back-end? To verify for 100% certain, for instance, that cosmiconfig doesn't get bundled for the client? Would be good to see that! |
Good call. Looking at the output of analyzing the build the sum of the front end bundle are as follow: Before (origin/develop): 448.93 KB The small increase is probably the result the changes (additions of getters/setters) and their usage. So nothing surprising there. Also there is no occurrences of |
…tions in that file.
Description
This PR is a re-work of the following PR. The predominant reason for iterating on the previous solution was to solve 2 main issues:
cosmiconfiglibrary included.Summary
The problem with the previous solution was that to used a single isomorphic function using a combination of webpack externals and evaluated
requirestatements to preventcosmiconfigfrom being bundled in client code and ensuring it's available for server code.Because V2 does not use externals, this solution wouldn't be easily implemented. Resulting in this solution where
cosmiconfigis only used on the server via the express app is made available to the rest of the application in a module context that can be accessed in both server and browser contexts.Bundle Size
See comment regarding effects on bundle size, showing that the client side consumed bundle is not effected from the addition of the
cosmiconfiglibrary that is only available on the server.Testing
A deployed version of this code can be seen here. In this deployment the javascript configuration is requiring 'site.json' information and exposing it at
__CONFIG__.app.sites. This isn't in this PR and was only used as a POC that you can use JS configuration files to their fullest.