Skip to content

fix: ensure x-app-uuid is present when using token #37

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

Merged
merged 5 commits into from
Jun 6, 2025

Conversation

mble-sfdc
Copy link
Contributor

The AppLink control plane requires this header to be present as part of validations, so lets ensure it is present.

Additionally, lets set a valid User-Agent header to indicate this is from the SDK.

Ref: W-18723611

The AppLink control plane requires this header to be present as part of
validations, so lets ensure it is present.

Additionally, lets set a valid User-Agent header to indicate this is
from the SDK.

Ref: W-18723611
@mble-sfdc mble-sfdc requested a review from a team as a code owner June 6, 2025 10:58
@mble-sfdc mble-sfdc force-pushed the mble-sfdc-add-app-uuid-header branch from 62b5ac3 to bac92e5 Compare June 6, 2025 11:25
const response = await fetch(url, opts);
const defaultOpts = {
headers: {
"User-Agent": `heroku-applink-node-sdk/1.0`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: we should be able to use fs to load package.json and get the version from there.

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 bends my brain a bit - just using something like:

    const pjson = fs.readFileSync(path.join(__dirname, "..", "package.json"), "utf8");
    const pkg = JSON.parse(pjson);

    const defaultOpts = {
      headers: {  
        "User-Agent": `heroku-applink-node-sdk/${pkg.version}`,
      },
    };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I forget __dirname isn't available in modules and have to do some hackery.

@@ -5,14 +5,23 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/

type AppUuid = string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious, why use a branded type?

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 is a Go habit of declaring "this thing really is a UUID instead of literally any string"

Copy link
Collaborator

@ekozilforce ekozilforce left a comment

Choose a reason for hiding this comment

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

Thank you for catching and fixing 🙇

@ekozilforce
Copy link
Collaborator

@brahyt-sf the test failures here look similar to the ones we are addressing in #36. I'm thinking since they are only failing on ubuntu we should go ahead and merge then try to resolve in the pr you have open. Wdyt?

@mble-sfdc mble-sfdc force-pushed the mble-sfdc-add-app-uuid-header branch from f933ee8 to 651e498 Compare June 6, 2025 13:59
@ekozilforce ekozilforce self-requested a review June 6, 2025 14:31
@mble-sfdc mble-sfdc merged commit 87c9251 into main Jun 6, 2025
4 of 8 checks passed
@ekozilforce ekozilforce mentioned this pull request Jun 6, 2025
@brahyt-sf
Copy link
Contributor

@brahyt-sf the test failures here look similar to the ones we are addressing in #36. I'm thinking since they are only failing on ubuntu we should go ahead and merge then try to resolve in the pr you have open. Wdyt?

@ekozilforce Yeah we can try that, I will address this failure in that PR. I also wanted to address some other things in that PR, its mistakenly Open and not in Draft.

@ekozilforce ekozilforce deleted the mble-sfdc-add-app-uuid-header branch June 9, 2025 18:05
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.

3 participants