fix(@stdlib/string/base/atob): guard global atob reference for Node <16#13453
Closed
Planeshifter wants to merge 2 commits into
Closed
fix(@stdlib/string/base/atob): guard global atob reference for Node <16#13453Planeshifter wants to merge 2 commits into
atob reference for Node <16#13453Planeshifter wants to merge 2 commits into
Conversation
… <16
The job `linux_test` (Node.js v12, v14) has failed on develop every
scheduled run for the past month with:
ReferenceError: atob is not defined
at .../lib/node_modules/@stdlib/string/base/atob/lib/global.js:23
Root cause: `lib/global.js` exported a bare reference to the global
`atob` identifier. Under `'use strict'`, referencing an undeclared
identifier throws a `ReferenceError` at evaluation time, and Node.js
only added `atob`/`btoa` as globals in v16.0.0. Because `lib/index.js`
unconditionally requires `./main.js` (which requires `./global.js`)
before ever checking `hasAtobSupport()`, simply requiring
`@stdlib/string/base/atob` crashed on any Node <16, defeating the
package's own polyfill fallback. The same unconditional require in
`test/test.main.js` crashed at require-time regardless of that test
file's `skip: !hasAtobSupport()` option, since `skip` only gates the
tape assertions, not the top-level `require`.
This commit guards the reference with `typeof atob === 'function'`,
mirroring the identical pattern already used by this package's own
dependency, `@stdlib/assert/has-atob-support/lib/atob.js`. `typeof`
on an undeclared identifier evaluates to `'undefined'` rather than
throwing, so the module now loads safely on Node <16 and correctly
falls back to the polyfill, while behavior on Node >=16 (where the
native `atob` is resolved) is unchanged.
Ref: https://github.com/stdlib-js/stdlib/actions/runs/29241732795
Contributor
Coverage Report
The above coverage report was generated for the changes in this PR. |
…bal.js` CI's `Lint Changed Files` check failed on this branch: the previous commit's `// eslint-disable-line stdlib/no-redeclare` was an unused directive (that rule never fires on this line, since `main` is declared, not `atob`), and the actual violation reported was `n/no-unsupported-features/node-builtins`, since `atob` is flagged as unsupported for the package's configured engines range (>=0.10.0). This commit points the disable comment at the correct rule and switches the fallback value from `void 0` to `null` to fully match the precedent in `@stdlib/assert/has-atob-support/lib/atob.js`. Behavior is unchanged; re-verified manually with and without a global `atob` present.
Member
|
I believe this is a duplicate PR. |
Member
Author
|
Confirmed — #13329 already proposes the identical fix (same file, same Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request:
linux_test(Node.js v12/v14 legs), which has failed on develop every scheduled run for the past month withReferenceError: atob is not definedinlib/node_modules/@stdlib/string/base/atob/lib/global.js. Root cause: the file exported a bare reference to the globalatobidentifier. Under'use strict', that throws aReferenceErrorat evaluation time on any JS engine lackingatobas a global — Node.js only addedatob/btoaas globals in v16.0.0. Becauselib/index.jsunconditionally requires./main.js(which requires./global.js) before ever checkinghasAtobSupport(), simplyrequire('@stdlib/string/base/atob')crashed on Node <16, defeating the package's own polyfill fallback.typeof atob === 'function', mirroring the identical pattern already used by this package's own dependency,@stdlib/assert/has-atob-support/lib/atob.js.typeofon an undeclared identifier evaluates to'undefined'rather than throwing, so the module now loads safely on Node <16 and correctly falls back to the polyfill. Behavior on Node >=16 is unchanged.Questions
No.
Other
Failing run: https://github.com/stdlib-js/stdlib/actions/runs/29241732795 (Node.js v12/v14 legs; the v16 leg in the same run fails separately, tracked in a different PR).
Symptom:
ReferenceError: atob is not definedatlib/node_modules/@stdlib/string/base/atob/lib/global.js:23.Validation: manually verified in a Node.js v22 sandbox (package installation was not available in this environment, so
make lint-pkg/the package test command could not be run directly):require('.../lib/global.js')withatobpresent returns the native function unchanged and decodes correctly.delete globalThis.atob; require('.../lib/global.js')no longer throws (previously threw immediately).delete globalThis.atob; require('@stdlib/string/base/atob')now correctly falls back to the polyfill and decodes'SGVsbG8sIHdvcmxk'to'Hello, world'.Reviewed in three passes: correctness (confirmed the fix eliminates the require-time throw on Node <16 while leaving Node >=16 behavior identical; confirmed no other unguarded global reference remains in the package's require chain), regression scope (confirmed this is the only file changed; confirmed no other package imports
lib/global.jsdirectly; confirmedmain.js's exported function is only ever invoked whenhasAtobSupport()is true, so behavior for real callers is unchanged), and style/conventions (confirmed the pattern matcheshas-atob-support/lib/atob.js; confirmed commit typefix(@stdlib/string/base/atob)is correct perdocs/style-guides/git/README.md).Reviewer notes
Non-blocking: the fallback value uses
void 0where the mirroredhas-atob-support/lib/atob.jsusesnull. Both are falsy and produce identical behavior on the unsupported path (the value is never read —main.js's exported function is never invoked whenhasAtobSupport()is false), so this was left as-is rather than forcing an exact literal match.Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
This PR was written primarily by Claude Code, investigating a GitHub Actions run failure and proposing a minimal source fix.
@stdlib-js/reviewers
Generated by Claude Code