Skip to content
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

feat: add pass headers guide #6960

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

rohrig
Copy link
Contributor

@rohrig rohrig commented Sep 19, 2023

πŸ”— Linked issue

❓ Type of change

  • [x ] πŸ“– Documentation (updates to the documentation, readme or JSDoc annotations)

πŸ“š Description

A guide to show integrators how to pass headers from the frontend through to the external services

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link
Contributor

@mattmaribojoc mattmaribojoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, very minor consistency suggestions

Copy link
Collaborator

@filrak filrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I got something wrong but since we're using Nuxt boilerplate shouldn't the guide focus more on how to modify the existing code for this particular use case rather than explaining how to set up the SDK itself?

filrak

This comment was marked as outdated.

@rohrig
Copy link
Contributor Author

rohrig commented Oct 2, 2023

Maybe I got something wrong but since we're using Nuxt boilerplate shouldn't the guide focus more on how to modify the existing code for this particular use case rather than explaining how to set up the SDK itself?

The guide does not use the nuxt 3 boilerplate. It uses the integrations boilerplate with the nuxt option.

@rohrig rohrig requested a review from filrak October 2, 2023 12:21
@rohrig rohrig force-pushed the feat/pass-custom-header-guide branch from f8756fd to b5e7541 Compare October 26, 2023 15:05
@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2023

[vuestorefront_vue-storefront_sdk] Kudos, SonarCloud Quality Gate passed!Β  Β  Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2023

[vuestorefront_vue-storefront_middleware] Kudos, SonarCloud Quality Gate passed!Β  Β  Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2023

[vuestorefront_vue-storefront_cli] Kudos, SonarCloud Quality Gate passed!Β  Β  Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Wojtek asked me to review the docs and correct it. This is not the final version yet.
@lsliwaradioluz
Copy link
Collaborator

@rohrig, here are my suggestions: https://github.com/vuestorefront/vue-storefront/pull/6996/files

LMK if you need any help with understanding those!

```javascript
import { initSDK, buildModule } from '@vue-storefront/sdk';
import { client, boilerplateModule, BoilerplateModuleType } from '../../packages/sdk/src';
import useHeaders from 'path-to-useHeaders-file'; // Replace with the correct path to the `useHeaders` composable.
Copy link
Contributor

@michaelKurowski michaelKurowski Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, who does provide the useHeaders, it's our product we should know the path to the composable if we provide it πŸ€”

Copy link
Contributor

@michaelKurowski michaelKurowski Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also feels like it's strictly vue specific since it mentions composables, is this what we want? I don't see any references to vue in the path of this file so it seems like you provide the general advice

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the customer is responsible for creation of the composable, but my question regarding the vue-centric docs remain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, if we can create a solution that framework agnostic that would be ideal. But I think given the differences in how Next and Nuxt handle ssr/state, I'm not sure that's possible. I'm not familiar enough with Next to know.

For context, the guide was made in response to an integrator's query that was using Nuxt. This started as general enablement and turned into a guide when several more folks asked the same question and all were using Nuxt.

If it's not possible to provide a framework agnostic solution, we can at least add examples in both support frameworks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok for both frameworks! :) Just including nuxt only solution felt a bit off

Copy link
Collaborator

@Fifciu Fifciu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this whole guide related to risks of shared client mentioned by @bartoszherba recently? Namely.


```javascript
import { initSDK, buildModule } from '@vue-storefront/sdk';
import { client, boilerplateModule, BoilerplateModuleType } from '../../packages/sdk/src';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it proper path?

```javascript
import { initSDK, buildModule } from '@vue-storefront/sdk';
import { client, boilerplateModule, BoilerplateModuleType } from '../../packages/sdk/src';
import useHeaders from 'path-to-useHeaders-file'; // Replace with the correct path to the `useHeaders` composable.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we provide it for our storefronts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it's below

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should make it more clear

Comment on lines +123 to +127
if (interceptorId !== null) {
client.interceptors.request.eject(interceptorId);
}

interceptorId = client.interceptors.request.use(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it will become flaky and unpredictable when there are many parallel connections to applications and shared client

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate? Not sure about the dangers if somebody uses the SDK server-side (e.g. in an Express application) but when it comes to the usage inside a storefront, how can SDK client be shared across multiple users running the storefront in their own browser? πŸ€”

Copy link
Collaborator

@lsliwaradioluz lsliwaradioluz Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm that's the case.

name: 'extension-dev-headers',
hooks: (req, res) => {
return {
afterCreate: ({ configuration }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why afterCreate instead of beforeCall?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it could be both

Comment on lines +156 to +197
```javascript
export default function () {
const serverHeaders = useRequestHeaders(['cookie']);
const devModeCookie = useCookie('dev-mode')?.value || '';

const state = useState<{ headerData: null | { isDevMode: string }, loading: boolean }>('headerState', () => ({
headerData: null,
loading: false,
}));

function addHeadersToState() {
getDevModeHeader(devModeCookie, serverHeaders.cookie || '');
}

function getHeaderObj(headers: string): { [key: string]: string } {
if (!headers) return {};

return headers.split(';').reduce<{ [key: string]: string }>((acc, item) => {
const parts = item.trim().split('=');
if (parts.length === 2) {
const [key, value] = parts;
acc[key] = value;
}
return acc;
}, {});
}

function getDevModeHeader(cookieHeader: string, serverHeader: string) {
if (process.server) {
const headerObj = getHeaderObj(serverHeader);
state.value.headerData = { isDevMode: headerObj['dev-mode'] || '' };
return
}

state.value.headerData = { isDevMode: cookieHeader };
}

return {
addHeadersToState,
...toRefs(state.value),
}
}
Copy link
Contributor

@michaelKurowski michaelKurowski Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we provide it ootb, instead of asking user to write it themselves? We even have the code already, you just wrote it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be great. But I created the example quickly and I'm not certain there aren't edge cases that need to be addressed. This PR is a bit old, so Unified wasn't on my radar when I wrote the guide and we didn't have a storefront in which I could add it. But now we do, so I think it would indeed be great to have this ootb in Unified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jagoral wdyt


```javascript
import { initSDK, buildModule } from '@vue-storefront/sdk';
import { client, boilerplateModule, BoilerplateModuleType } from '../../packages/sdk/src';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, where is the client coming from, is it exposed by every SDK module? Is it coordinated with the unified team, do they expose it as well the same way?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import path should be less ambigious. But long story short: the client is coming from an SDK module (e.g. sapcc-sdk), every module exports the client. Not sure this is going to be the case in a few weeks/months (considering the ongoing discussion initiated by @bartoszherba) but, for now, that's the state of things.

Copy link
Contributor

@michaelKurowski michaelKurowski Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really think this doc should be agreed upon with unified. Consider how confused a customer on unified storefront would be after seeing these docs if unified module doesn't expose this (and honestly they probably couldn't know before that they should).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guide sat on the back burner for a while. Given the change in landscape since it's inception I agree there should be a consensus and standard way of doing this. RE: comments about ootb, I think it would be sensible for for the Unified team to implement the boilerplate to allow passing custom headers and allow the end dev to use a one-liner, if possible, to add headers to requests. The guide for an ootb solution would be a separate guide though.

we still have two scenarios:

  1. Projects using Unified: for those we can have an ootb solution and a separate guide
  2. Non-Unified: this sort of guide will still be necessary.

Copy link
Collaborator

@WojtekTheWebDev WojtekTheWebDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exporting client as our singleton instance became problematic recently and I think we should discuss it during the refi before releasing this guide

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants