Enable PWA private client via boolean #1704
Merged
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
|
First of all, double checking that the target branch is correct. This PR is to be merged into |
|
@vmarta The intent is that this will be merged eventually into the feature/pwa-kit-slas-private-support branch. I currently have this pointing to the the private-client-hybrid branch since these changes were originally made on top of #1696 (which will also be merged into feature/pwa-kit-slas-private-support). I could point the PR directly to feature/pwa-kit-slas-private-support but then you'll see the contents of #1696 also. |
vmarta
reviewed
Mar 22, 2024
Comment on lines
221
to
225
| this.clientSecretPlaceholder = config.enablePrivateClient | ||
| ? SLAS_SECRET_PLACEHOLDER // PWA proxy is enabled, assume project is PWA | ||
| : config.clientSecret | ||
| ? config.clientSecret | ||
| : '' |
There was a problem hiding this comment.
For more readable code:
- Without the chaining, we can simplify it to this.
- How about considering the variable name to be
this.clientSecret? It's like saying the client secret can be either a placeholder or the real secret.
Suggested change
| this.clientSecretPlaceholder = config.enablePrivateClient | |
| ? SLAS_SECRET_PLACEHOLDER // PWA proxy is enabled, assume project is PWA | |
| : config.clientSecret | |
| ? config.clientSecret | |
| : '' | |
| this.clientSecret = config.enablePrivateClient | |
| ? SLAS_SECRET_PLACEHOLDER // PWA proxy is enabled, assume project is PWA | |
| : config.clientSecret || '' |
There was a problem hiding this comment.
For non-pwa-kit users, how will they know to pass in clientSecret but not the enablePrivateClient? 🤔
There was a problem hiding this comment.
For non-pwa-kit users, how will they know to pass in
clientSecretbut not theenablePrivateClient? 🤔
What if they pass in both enablePrivateClient and the clientSecret? So then our code can be like this:
this.clientSecret = config.enablePrivateClient
? config.clientSecret || SLAS_SECRET_PLACEHOLDER
: ''There was a problem hiding this comment.
We've updated this to be the following:
this.clientSecret = config.enablePWAKitPrivateClient
? // PWA proxy is enabled, assume project is PWA and that the proxy will handle setting the secret
// We can pass any truthy value here to satisfy commerce-sdk-isomorphic requirements
SLAS_SECRET_PLACEHOLDER
: config.clientSecret || ''
If enablePWAKitPrivateClient is true, it takes precedence over clientSecret.
We've renamed the first prop to enablePWAKitPrivateClient since it is what PWA users will toggle and to make it more clear that non-PWA users should not enable this.
clientSecret is only used by non-PWA Kit projects.
vmarta
approved these changes
Mar 25, 2024
alexvuong
reviewed
Mar 25, 2024
alexvuong
reviewed
Mar 25, 2024
| clientSecret="_PLACEHOLDER_PROXY-PWA_KIT_SLAS_CLIENT_SECRET" | ||
| // Uncomment 'enablePWAKitPrivateClient' to use SLAS private client login flows. | ||
| // Make sure to also enable useSLASPrivateClient in ssr.js when enabling this setting. | ||
| enablePWAKitPrivateClient={true} |
There was a problem hiding this comment.
this is enabled by default when a project is generated with private slas. Tte comment is telling the other way around when enablePWAKitPrivateClient={true} is commented out.
There was a problem hiding this comment.
Yes, in the templates used by pwa-kit-create-app we set enablePWAKitPrivateClient and don't comment it out.
The comment is there to keep the pwa-kit-create-app files more in sync with the one in template-retail-react-app.
vmarta
approved these changes
Mar 26, 2024
vcua-mobify
changed the base branch from
private-client-hybrid
to
feature/pwa-kit-slas-private-support
…holder Signed-off-by: vcua-mobify <[email protected]>
vcua-mobify
merged commit 0600f28
into
feature/pwa-kit-slas-private-support
28 checks passed
vcua-mobify
added a commit
that referenced
this pull request
Apr 2, 2024
* add support private slas flow by using place holder value * revert ssr js in template * revert code in template * remove console * minor fix * add test for refreshToken * allow secret passed in as prop * add silenceWarning prop and readme * add changelog * PR feedback * PR feedback * PR feedback * PR feedback * PR feedback * lint * @W-14758629@ - Add a custom proxy handler for SLAS private clients (#1664) * Allow objects in commerceapi path * Add custom endpoint for SLAS. Works on public client * Get private client working * Remove console log * A more generic header replacement * Slas credentials from env vars * Add exclusions for some endpoints * Custom middleware in ssr.js * Add dependency * Private SLAS client handler in runtime * Update commerce-sdk-react with private client endpoint * Cleanup * Public client by default * more cleanup * Fix existing tests * Runtime tests- WIP * WIP test * Apply some PR feedback * Small refactor * Working tests * add getSlasEndpoint test * Lint * Change endpoint path and other minor adjustments * Don't start server if env var not set * Fix flaky test * Add option for customizing more endpoints with private client * Rename client secret env var * Bump up a test timeout * @W-14810956@ Update generator with options for slas private client (#1683) * Allow objects in commerceapi path * Add custom endpoint for SLAS. Works on public client * Get private client working * Remove console log * add private slas/private question * add private slas/private question * A more generic header replacement * Slas credentials from env vars * Add exclusions for some endpoints * add private slas/private question * Custom middleware in ssr.js * Add dependency * Private SLAS client handler in runtime * Update commerce-sdk-react with private client endpoint * Cleanup * Public client by default * more cleanup * Fix existing tests * Runtime tests- WIP * WIP test * Apply some PR feedback * Small refactor * Working tests * add getSlasEndpoint test * Lint * Change endpoint path and other minor adjustments * Don't start server if env var not set * Fix flaky test * Add option for customizing more endpoints with private client * Rename client secret env var * Bump up a test timeout * Update generator * Lint * Fix lint in generated projects * Add comments on the original app config file * Add templates for non-extensible projects * More detailed developer note --------- Co-authored-by: Alex Vuong <[email protected]> * @W-14758629@ Private client handle missing env var (#1679) * Allow objects in commerceapi path * Add custom endpoint for SLAS. Works on public client * Get private client working * Remove console log * add private slas/private question * add private slas/private question * A more generic header replacement * Slas credentials from env vars * Add exclusions for some endpoints * add private slas/private question * Custom middleware in ssr.js * Add dependency * Private SLAS client handler in runtime * Update commerce-sdk-react with private client endpoint * Cleanup * Public client by default * more cleanup * Fix existing tests * Runtime tests- WIP * WIP test * Apply some PR feedback * Small refactor * Working tests * add getSlasEndpoint test * Lint * Change endpoint path and other minor adjustments * Don't start server if env var not set * Fix flaky test * Add option for customizing more endpoints with private client * Rename client secret env var * Bump up a test timeout * Update generator * Lint * Fix lint in generated projects * Add comments on the original app config file * Add templates for non-extensible projects * Improve missing env var handling in remote environments * More detailed developer note * Remove brackets --------- Co-authored-by: Alex Vuong <[email protected]> * Private SLAS default to yes * @W-14959704@ Set private client proxy headers (#1690) * Add header injection to custom proxy * Refactor to remove duplicate code * Update private client preset config * Update e2e workflow to test for private client apps * Add conditional destructuring * Add condition to evaluate generator prompts * Add null checks to cli prompt destructuring * remove console logs * Disable npm cache for private clients * Clear verdaccio storage in e2e test setup * lerna force publish all packages * lerna force publish all packages * lerna force publish all packages * remove force publish * Bump versions * Remove steps to clear verdaccio storage * Test slack notification for private client * Add responses for private client to generator test * Restore notification on schedule * Run private client generator linearly * @W-15163165@ Reduce session churn / fix SFRA -> PWA session share on private (#1696) * Replace SFRA->PWA session handoff logic * Merge branch 'feature/pwa-kit-slas-private-support' into private-client-hybrid * Remove console.logs * Adjust cookie name * Code refactor * Return empty string * Tests * Rework suffix of cookie chunks * Update more local storage values on SFRA token handoff * Test local storage update * Remove refresh token copies from local store * Remove refresh token copy references in test * Code cleanup * Enable PWA private client via boolean (#1704) * Replace SFRA->PWA session handoff logic * Merge branch 'feature/pwa-kit-slas-private-support' into private-client-hybrid * Remove console.logs * Adjust cookie name * Code refactor * Return empty string * Tests * Rework suffix of cookie chunks * Update more local storage values on SFRA token handoff * Test local storage update * Remove refresh token copies from local store * Remove refresh token copy references in test * Enable PWA private client via boolean * Disable private client in template and update comments * Move placeholder to constants * Apply feedback * Update comment * Update comments --------- Signed-off-by: vcua-mobify <[email protected]> * E2E move preset definition to config * Fix missing preset id * Remove missing property from preset * Remove console logs * Update changelogs * Add preset for private client hybrid CI * Update name --------- Signed-off-by: vcua-mobify <[email protected]> Co-authored-by: Alex Vuong <[email protected]> Co-authored-by: Ben Chypak <[email protected]> Co-authored-by: Jainam Tushar Sheth <[email protected]> Co-authored-by: Jainam Sheth <[email protected]>
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.
Small improvement to allow the use of a boolean to enable SLAS Private Client mode in commerce-sdk-react rather than a truthy string.
This change keeps open the option for non-PWA consumers of commerce-sdk-react to provide their own client secret.
Testing these changes:
In template-retail-react-app enable private clients by:
usePrivateClient: truein ssr.jsenablePrivateClient={true}in app-config/index.jsx CommerceAPIProviderMake sure the above is automatically done in a generated app.