Skip to content

Conversation

KijongHan
Copy link

@KijongHan KijongHan commented Sep 21, 2025

Description

Added your feature that allows adding custom headers based on url pattern matching, as per discussion here

  • add optional options parameter to Mapbox.addCustomHeader with optional urlPattern key which defines the regex string for when there is a match against the request.url it will add the custom header value header and value.
  • There is no change to the default behaviour or method signature.
  • If there is an error with converting to regex pattern it will default to not adding the associated header to the request

Checklist

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder
  • I have tested the new feature on /example app.
    • In V11 mode/ios
    • In New Architecture mode/ios
    • In V11 mode/android
    • In New Architecture mode/android
  • I added/updated a sample - if a new feature was implemented (/example)

Screenshot OR Video

Screenshot 2025-09-21 at 7 57 24 PM (only Custom-Header and Mapbox-Api-Header-Value added as per `App.js` example)

…ttern to add header only when there is a regex pattern match against urlPattern

- Add example usage of addCustomHeader to example App.js and disable escape character check for regex characters
@KijongHan
Copy link
Author

I know contributions guidelines detail how to generate components docs but it doesn't look like Mapbox.md and CustomHttpHeaders.md are hooked up to auto generate based on code block comments? Have manually updated those docs in PR

@mfazekas
Copy link
Contributor

I know contributions guidelines detail how to generate components docs but it doesn't look like Mapbox.md and CustomHttpHeaders.md are hooked up to auto generate based on code block comments? Have manually updated those docs in PR

Yes Mapbox.md and CustomHttpHeaders.md not generated

Copy link
Contributor

@mfazekas mfazekas left a comment

Choose a reason for hiding this comment

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

👍 awesome, thanks much for working on it

headers[entry.key] = entry.value.headerValue
}
} catch (e: PatternSyntaxException) {
Log.w(LOG_TAG, e.localizedMessage ?: "Error converting ${options?.urlPattern} to regex")
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer Logger.e as that's visible in react-native side as well

Also it would be better to convert to regex in add, as we have a few adds and a lot of tests

// Check if a URL pattern exists.
if let pattern = options.urlPattern {
do {
let regex = try NSRegularExpression(pattern: pattern)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as android: store as regexp in add and just test here

addCustomHeader(
headerName: string,
headerValue: string,
options?: { urlPattern?: string },
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call urlRegexp as we're expecting a regular epxression?

Copy link
Author

Choose a reason for hiding this comment

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

sounds good, updated

… modules to caller scope for better defined boundaries

- rename urlPattern to urlRegexp
@KijongHan KijongHan force-pushed the feature/add-options-parameter-to-add-custom-header branch from 44e4145 to 83c3dd8 Compare September 21, 2025 20:15
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens September 21, 2025 20:16 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens September 21, 2025 20:16 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 1, 2025 09:50 — with GitHub Actions Failure
@KijongHan KijongHan requested a review from mfazekas October 1, 2025 09:50
@KijongHan
Copy link
Author

hey @mfazekas - I had to try a different approach and added a new method addCustomHeaderWithOptions as the nullable options parameter from Kotlin wasn't being surfaced to the javascript RBMXModule (it was still expecting 3 explicit parameters and would break backwards compatibility) - have addressed your other PR comments

@mfazekas
Copy link
Contributor

Thanks much looks good to me. Not sure if it's possible to add a new TypeScript method to RNMBXModule, which would call the appropriate native method.

hey @mfazekas - I had to try a different approach and added a new method addCustomHeaderWithOptions as the nullable options parameter from Kotlin wasn't being surfaced to the javascript RBMXModule (it was still expecting 3 explicit parameters and would break backwards compatibility) - have addressed your other PR comments

@KijongHan
Copy link
Author

Thanks much looks good to me. Not sure if it's possible to add a new TypeScript method to RNMBXModule, which would call the appropriate native method.

hey @mfazekas - I had to try a different approach and added a new method addCustomHeaderWithOptions as the nullable options parameter from Kotlin wasn't being surfaced to the javascript RBMXModule (it was still expecting 3 explicit parameters and would break backwards compatibility) - have addressed your other PR comments

hey when I tried doing this the method signature in the RBMXModule javascript was still expecting a match against the native layer, so when I tried implementing a addCustomHeader method export with optional param that calls into addCustomHeaderWithOptions it was still expecting addCustomHeader with 2 arguments

Copy link
Contributor

@mfazekas mfazekas left a comment

Choose a reason for hiding this comment

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

Pls fix CI, otherwise looks good to me

.eslintrc.js Outdated
'no-return-assign': 0,
'no-underscore-dangle': [0],
'no-await-in-loop': 0,
'no-useless-escape': ['error', { allowRegexCharacters: ['-'] }],
Copy link
Contributor

Choose a reason for hiding this comment

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

CI fails:

Error: .eslintrc.js:
	Configuration for rule "no-useless-escape" is invalid:
	Value [{"allowRegexCharacters":["-"]}] should NOT have more than 0 items.

Copy link
Author

Choose a reason for hiding this comment

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

oops mb bad commit - was testing this out locally

Copy link
Author

Choose a reason for hiding this comment

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

hey @mfazekas - have fixed eslint and unit tests for CI, I think the other tests that fail are related to requiring mapbox tokens? not sure

@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 03:54 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 03:54 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 03:54 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 03:54 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 03:54 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 03:54 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 03:54 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 22:57 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 22:57 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 22:57 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 22:57 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 22:57 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 22:57 — with GitHub Actions Failure
@KijongHan KijongHan had a problem deploying to CI with Mapbox Tokens October 15, 2025 22:57 — with GitHub Actions Failure
@KijongHan KijongHan temporarily deployed to CI with Mapbox Tokens October 19, 2025 19:22 — with GitHub Actions Inactive
@KijongHan KijongHan temporarily deployed to CI with Mapbox Tokens October 19, 2025 19:22 — with GitHub Actions Inactive
@KijongHan KijongHan temporarily deployed to CI with Mapbox Tokens October 19, 2025 19:22 — with GitHub Actions Inactive
@KijongHan KijongHan temporarily deployed to CI with Mapbox Tokens October 19, 2025 19:22 — with GitHub Actions Inactive
@KijongHan KijongHan temporarily deployed to CI with Mapbox Tokens October 19, 2025 19:22 — with GitHub Actions Inactive
@KijongHan KijongHan deployed to CI with Mapbox Tokens October 19, 2025 19:22 — with GitHub Actions Active
@KijongHan KijongHan temporarily deployed to CI with Mapbox Tokens October 19, 2025 19:22 — with GitHub Actions Inactive
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.

2 participants