Add targeting snippets #3944
Add targeting snippets #3944
Conversation
|
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 2708648 | Triggered | Generic High Entropy Secret | d2ec816 | examples/angular-gen2/src/environments/environment.ts | View secret |
| 11707119 | Triggered | Generic High Entropy Secret | d2ec816 | packages/sdks/snippets/react/src/routes/query-cheatsheet/index.tsx | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
View your CI Pipeline Execution ↗ for commit d2ec816.
☁️ Nx Cloud last updated this comment at |
| @@ -0,0 +1,9 @@ | |||
| import { builder } from '@builder.io/react'; | |||
|
|
|||
| export default async ({ model, urlPath }: { model: string; urlPath: string }) => | |||
There was a problem hiding this comment.
- unnecessary async
- for consistency all requests fns should have the same name. I see some don't have a name at all, and some are called
request. I findrequestto be a bit of a special keyword in JavaScript land. I'd opt for a name liketargettedRequest
There was a problem hiding this comment.
Thank you, @samijaber. I've updated the code to remove the last few async keywords. I've also used the GetContentOptions type in Gen 1 now that I know where it is.
- Remove unnecessary async keywords - Rename functions to all be the same - Include GetContentOptions type for Gen 1 React
There was a problem hiding this comment.
Just took a look at the doc: https://www.builder.io/c/docs/targeting-cheatsheet#examples-of-custom-targeting-attributes
Are we reworking the doc completely? Right now that page uses builder.setUserAttributes() to illustrate how those attributes can be set, whereas this PR is using builder.get() calls
| export default function targetedRequest({ model, url }: GetContentOptions) { | ||
| return builder.get(model!, { | ||
| userAttributes: { | ||
| audience: ['mens-fashion'], | ||
| urlPath: url, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
-
Recently added
!to the list of things we want to avoid in our docs: https://www.notion.so/builderio/Docs-code-snippet-guidelines-1283d7274be580d1a96cdafcf6cab708?pvs=4#1ab3d7274be58085b038e2b7a609c435 -
GetContentOptionsis specific to the second argument ofbuilder.get, so it's more consistent to use it only for that part -
urlin gen1 can be used to replaceuserAttributes.urlPath, as per its code comment:
Putting all that together:
| export default function targetedRequest({ model, url }: GetContentOptions) { | |
| return builder.get(model!, { | |
| userAttributes: { | |
| audience: ['mens-fashion'], | |
| urlPath: url, | |
| }, | |
| }); | |
| } | |
| export default function targetedRequest(modelName: string, options: GetContentOptions) { | |
| return builder.get(modelName, { | |
| userAttributes: { | |
| audience: ['mens-fashion'], | |
| }, | |
| ...options, | |
| }); | |
| } |
| import { fetchOneEntry, GetContentOptions } from '@builder.io/sdk-react'; | ||
|
|
||
| export default function targetedRequest( | ||
| urlPath: string, | ||
| { apiKey, model }: GetContentOptions | ||
| ) { | ||
| return fetchOneEntry({ | ||
| apiKey, | ||
| model, | ||
| userAttributes: { | ||
| isLoggedIn: true, | ||
| urlPath, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The arguments of targetedRequest add a layer of indirection that can complicate the code snippet for the end-user.
I would stick with a philosophy of "spread everything" to keep this wrapper function as unobtrusive as possible:
| import { fetchOneEntry, GetContentOptions } from '@builder.io/sdk-react'; | |
| export default function targetedRequest( | |
| urlPath: string, | |
| { apiKey, model }: GetContentOptions | |
| ) { | |
| return fetchOneEntry({ | |
| apiKey, | |
| model, | |
| userAttributes: { | |
| isLoggedIn: true, | |
| urlPath, | |
| }, | |
| }); | |
| } | |
| import { fetchOneEntry, GetContentOptions } from '@builder.io/sdk-react'; | |
| export default function targetedRequest(options: GetContentOptions) { | |
| return fetchOneEntry({ | |
| ...options, | |
| userAttributes: { | |
| isLoggedIn: true, | |
| ...options.userAttributes, | |
| }, | |
| }); | |
| } |
There was a problem hiding this comment.
@samijaber Is your assumption then that the arguments provided to targetedRequest() will look something like this?
const options = {
apiKey: BUILDER_API_KEY,
model: MODEL,
userAttributes: {
urlPath: window.location.pathname,
},
};As someone new to the code, I feel like having urlPath explicit within the code snippet feels important as the request will not work without it. Do you think something like this is too verbose?
import { fetchOneEntry, GetContentOptions } from '@builder.io/sdk-react';
export default function targetedRequest(options: GetContentOptions) {
return fetchOneEntry({
...options,
userAttributes: {
isLoggedIn: true,
urlPath: options.userAttributes?.urlPath,
...options.userAttributes, // Include additional userAttributes
},
});
}There was a problem hiding this comment.
Is your assumption then that the arguments provided to
targetedRequest()will look something like this?
Yeah, I want to avoid creating an extra abstraction. I want targetedRequest to have the same arguments as fetchOneEntry.
True that urlPath needs to exist (in most cases) for a response to return to us, but given that the focus of the snippet is setting/passing user attributes, I think it's OK if we imply "the spread arguments will contain everything else that you need"
I am trying to make some significant changes to the doc. There are more details in the Jira ticket, but in short I would like to separate out the code blocks from the more generalized information about targeting. I also worked on the Querying Cheatsheet, which has a similar name but is structured very differently. This page seems focused around being able to quickly find the code snippet you need and copy it to your code, while the current Targeting Cheatsheet acts a bit more like a tutorial. If you disagree with this approach, however, please feel free to let me know. I'm very much still getting a handle on all the documentation. |
There was a problem hiding this comment.
hmm, i see these as two separate things:
-
setting a user attribute:
- gen1:
builder.setUserAttribute() - gen2:
setClientUserAttributes()
- gen1:
-
usinga user attribute when fetch content:userAttributes: {}argument tobuilder.getandfetchOneEntry/fetchEntries
The doc currently has the former, you're replacing it with a latter. I think we'd like both ideally!
| } | ||
| } | ||
| fetchContent(); | ||
| }, [window.location.pathname]); |
There was a problem hiding this comment.
heads up that useEffect cannot listen to changes in window.location.pathname.
this is used for our own testing, so as long as you get the desired behaviour it's fine i guess, but more of a heads up in general
There was a problem hiding this comment.
Thanks, I did not know this but it makes sense! 👍
- Rework function signatures
- Add example cases for using Gen 1 and Gen 2 set attributes - Add tests that succeed for Gen 1 but fail for Gen 2
| export default function setTargetingAttributes() { | ||
| setClientUserAttributes({ | ||
| device: 'desktop', | ||
| audience: ['recent-shopper', 'womens-fashion'], | ||
| }); | ||
| } |
There was a problem hiding this comment.
@samijaber Here is the option that's failing. I am using this function and file similarly in the Gen 1 directory.
This function does set the cookies appropriately but fetchOneEntry() does not seem to pick up the details in the cookie.
Description
Adds snippets to be used within the Targeting Cheatsheet doc. The only files that will be linked within the docs are the non-index files within the
targeted-page/directory.