feat: enhance global configuration handling and improve attribute retrieval logic#4198
feat: enhance global configuration handling and improve attribute retrieval logic#4198Yashh56 wants to merge 4 commits into
Conversation
|
@Yashh56 is attempting to deploy a commit to the Umami Software Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR adds a The null-stringification bug from the prior review thread is fully resolved ( Confidence Score: 4/5Safe to merge after clarifying whether the recorder's The core logic is correct and the two previously flagged issues are partially or fully resolved. The recorder/tracker inconsistency is the main unresolved concern — it silently disables recordings for src/recorder/index.js — early-exit guard on line 6 was not updated to mirror the tracker's Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Script loads] --> B{currentScript present?}
B -- Yes --> C[Bind attr to currentScript]
B -- No --> D{umamiConfig.websiteId set?}
D -- No --> E[Return / abort]
D -- Yes --> F[attr returns null for all]
C --> G[config lookup]
F --> G
G --> H{globalConfig camelKey != null?}
H -- Yes --> I{before-send and typeof fn?}
I -- Yes --> J[Return function directly]
I -- No --> K{Array value?}
K -- Yes --> L[join with comma]
K -- No --> M[String coerce]
H -- No --> N[Read data-attr from script tag]
J --> O[Extract website, hostUrl, beforeSend, etc]
L --> O
M --> O
N --> O
O --> P[Tracker initializes and sends events]
Reviews (4): Last reviewed commit: "Refactor global configuration handling t..." | Re-trigger Greptile |
|
Nice work on this — the overall approach looks solid and covers the main use case from the feature request well. A few follow-ups I’d suggest: 1. domains as an array gets silently coercedThe feature request example used domains: ['mywebsite.com'], but String(['a.com', 'b.com']) happens to work only because arrays stringify comma-separated. Entries containing commas or whitespace would break the split. Either handle arrays explicitly in config(): …or document clearly that domains must be a comma-separated string even when set via umamiConfig. 2. beforeSend still expects a string referenceThe tracker reads window[beforeSend], so umamiConfig.beforeSend has to be a function name as a string, not the function itself. That’s a GTM sandbox hurdle — setInWindow can set the name, but getting the callback function onto window from a Custom Template is awkward. Not blocking for most GTM Template use cases (most people won’t need beforeSend), but ideally globalConfig.beforeSend would also accept a function directly, e.g.: 3. Recorder needs the same treatmentrecorder.js (Session Replay) is a separate script with its own data-* attributes (data-sample-rate, data-mask-level, data-max-duration) and currently has the exact same GTM Custom Template sandbox limitation this PR fixes for the tracker. Covering both would enable a complete “Umami via GTM” setup — tracker + recorder. Happy to open a follow-up PR specifically for the recorder if you’d prefer to keep this one focused. |
… and improve attribute retrieval logic
…nd enhance function retrieval logic
|
Hey @mikecao and @franciscao633, could you please take a look and share your feedback? |
This pull request updates the tracker initialization logic to allow configuration via a global
umamiConfigobject in addition to the existing method of usingdata-attributes on the script tag. This makes it possible to initialize the tracker even when the script tag is not present, and allows for more flexible configuration.Key changes:
Configuration improvements:
Added support for a global
umamiConfigobject to provide configuration values (such aswebsiteIdandhostUrl) as an alternative todata-attributes on the script tag. This enables initialization without requiring the script tag to be present. [1] [2]Updated the
configfunction to prioritize values fromumamiConfig, falling back todata-attributes if not found. Also added a utility to convert config keys from kebab-case to camelCase for compatibility.Robustness improvements:
currentScriptis not available, preventing errors during initialization and when constructing the API host URL. [1] [2]Closes: #4177