Repository navigation
fix(formatter): read the platform defensively, and guard the platform helpers - #281
Merged
Merged
Conversation
… helpers `newLine()`, `isWindows()` and `isLinux()` read `process.platform` directly. That is fine in Node and a ReferenceError in a browser, and the browser case is not hypothetical: `newLine` is part of the formatter, which runs in browsers to build request bodies. Found downstream. The adapter's browser build has been carrying a Node stdlib polyfill tree - shimmed crypto, stream, os, path and buffer - whose only job in practice was to answer reads like this one. Guarding them here lets that tree go: with these three functions fixed, the adapter's browser bundle needs a twenty-line process stub rather than thousands of lines of bundled Node internals. - `newLine()` returns the unix ending where there is no process. - `isWindows()` and `isLinux()` answer false there. Both are exported from the package root and callable from browser code. - The FIXME about `isWindows` is resolved by keeping the formatter self-contained rather than importing it: that module imports `path` and is loaded from the root, which is what pulled Logger in and broke cli.spec.ts. Tests, because the failure mode is invisible in Node: each spec now deletes the process global and asserts the fallback, and restores it afterwards. The existing specs mutated `process.platform` without restoring, so they are pinned down the same way. Verified by reintroducing the bug: the new case fails with `ReferenceError: process is not defined`, and passes with the guard in place.
…advisories The toolchain audit fails on three handlebars advisories, all fixed in 4.7.10: critical GHSA-8r5x-fm3f-whwj JavaScript Injection via AST Type Confusion critical GHSA-p8wg-vrv2-v86f JavaScript Injection via Own Property Check Bypass moderate GHSA-xw65-4hp5-5hc7 JavaScript Injection via Unsafe Inline Embedding Not this PR's subject matter, but CI cannot be green without it, and the advisories are new: main's last run of this workflow passed before they were published. `release/audit.mjs` says fix it or exempt it with a reason, and 4.7.10 is available, so this fixes it rather than exempting a critical injection. release/package-lock.json: handlebars 4.7.9 -> 4.7.10. Verified: npm ci --prefix release --ignore-scripts, then node release/audit.mjs - "no advisory in the release toolchain outside the documented exemptions (13 exempt)", exit 0.
sasjs-gha Bot
pushed a commit
that referenced
this pull request
Oct 15, 2026
## [3.7.3](v3.7.2...v3.7.3) (2026-10-15) ### Bug Fixes * **formatter:** read the platform defensively, and guard the platform helpers ([#281](#281)) ([cf9efb4](cf9efb4))
|
🎉 This PR is included in version 3.7.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
sasjs-dev Bot
added a commit
to sasjs/adapter
that referenced
this pull request
Oct 15, 2026
…read @sasjs/utils 3.7.3 guards the process.platform reads in newLine, isWindows and isLinux (sasjs/utils#281), which is the downstream half of this branch: the adapter's browser bundle only needed a Node stdlib polyfill tree because those reads threw in a browser. 3.7.1 -> 3.7.3, skipping 3.7.2. What that changes here: the only process.platform read left in the browser bundle is inside utils' own guard, so the stub no longer needs the field for correctness. It keeps it, and its comment now names the reads that remain - readable-stream's process.nextTick, util-deprecate's process.env and deprecation flags, and a bundled asap helper that falls back to nextTick when queueMicrotask is missing. Verification - npm run build exits 0; npm test passes 52 suites / 347 tests. - Packed: 231 files / 419 kB, carrying @sasjs/utils 3.7.3. - Markers in the shipped JavaScript are down to webpack's runtime global helper (`new Function`, once per browser bundle). No process.binding shim, no safe-buffer, no os.homedir, no Node stdlib polyfill packages, and none in the compiled tree under the package root. - The browser bundle still loads and constructs an instance in a VM context with no process global (47 instance methods). - Node entry parity is unchanged: 25 named exports and 48 prototype methods, matching the 4.19.4 bundle. CI reruns on this push, so the adapter-tests deployables for the browser run are rebuilt from this commit.
allanbowe
pushed a commit
to sasjs/adapter
that referenced
this pull request
Oct 16, 2026
… build in dist (#932) * build(package): ship the adapter as compiled modules, with the browser build in dist Spike, not for merge yet. It answers one question: what does the package look like if the opaque bundles go away? npm's publish-time scanner has removed five versions of this package (5.0.0 through 5.0.4), including 5.0.4, which carried the dual-use declaration the policy asks for. The scan does not say what it objects to, so this branch removes the shape most likely to attract it - large opaque bundles of bundled third-party code - while keeping every consumption path that consumers rely on. The package - The root is compiled CommonJS from tsc: index.js plus the module tree, with declarations. It was a 1.33 MB webpack bundle of bundled third-party code. - The browser build moves to dist/sasjs-adapter.js, and the SAS 9 build to dist/minified_sas9.js. It is no longer the package entry. - package.json gains a `browser` field pointing at dist/sasjs-adapter.js, so bundler consumers keep working without configuration. The root entry cannot serve them directly: six source files import `https` statically and several use `process.logger` bare, which is why the browser bundle needs its polyfills. - @sasjs/adapter/node is a re-export of the root rather than a second bundle. The CLI imports it and nothing imports below it, so a shim is enough and the tree is not duplicated. - README install step 2 now names dist/sasjs-adapter.min.js. The old link pointed at index.min.js, which jsDelivr builds by minifying index.js; the file it names has moved. - The files allowlist is narrowed to the trees that actually ship. The first attempt used `**/*.js` and `**/*.d.ts` and the release pack-check caught it reaching 4988 files in release/node_modules - the exact leak that guard exists for, and worth recording as the guard working. What it measures packed tarball 226 files / 739 kB -> 229 files / 521 kB Node surface 3 bundles -> 114 compiled modules + 2 browser files largest js 1.33 MB (bundle) -> 1.33 MB (dist/sasjs-adapter.js) Scan-relevant markers now appear only in dist/, nowhere in the package's Node code: before after where after new Function 2 2 dist/sasjs-adapter.js, dist/minified_sas9.js process.binding shim 3 2 dist/sasjs-adapter.js, dist/minified_sas9.js safe-buffer 1 1 dist/sasjs-adapter.js os.homedir() 1 1 dist/sasjs-adapter.js process.env 10 10 dist/sasjs-adapter.js, dist/minified_sas9.js Two experiments, both recorded because they bound the next step - output.globalObject: 'globalThis' does not remove `new Function`. The helper is webpack's runtime global (`__webpack_require__.g`), emitted whenever the bundle needs the global object, and its template keeps the `new Function('return this')()` fallback. Removing it means changing the browser bundler, which brings its own polyfill handling. - Dropping nodePolyfillPlugin fails the build outright: @fast-csv/format and @sasjs/utils/file reference `stream` and `path`. The polyfills are load-bearing for the browser bundle as it is currently composed, so de-polyfilling means unbundling those dependencies from the browser path - a separate project, and one that needs a browser run against a SAS server to validate. Verification - npm run build exits 0 and produces the layout above. - npm test: 52 suites, 347 tests, all passing. - release/pack-check.mjs passes. In CI it runs after publishInit, when the tree is at the package root, which is the context it is written for. - API parity: the packed root entry and the packed node entry both expose the same 25 named exports and 48 prototype methods, and both match the old 4.19.4 bundle key for key. - prettier passes on the touched files, and the browser field points at a file that exists in the tarball. What is left, and why it is a separate decision Making the source isomorphic - guarding process access, moving the static `https` imports into node-only modules - would let the root entry serve browsers directly and drop the browser polyfills entirely. That changes runtime behaviour in code paths the unit tests do not exercise, so it needs the adapter-tests deployable run against a real SAS server before it can be trusted. Nothing here is published. The branch exists so the shape can be reviewed and, if the hypothesis is worth a publish, staged once the appeal has been answered or abandoned. * build(package): make the source isomorphic, and drop the Node polyfills Second increment on the spike branch. The package root is already compiled modules rather than bundles; this makes the source run in both environments so the browser bundle no longer needs shimmed Node internals. npm's publish-time scanner has removed five versions of this package (5.0.0 through 5.0.4), including 5.0.4, which carried the dual-use declaration the policy asks for. The scan does not say what it objects to, so this removes the bundled polyfill tree - thousands of lines of shimmed Node internals - that the browser bundle used to carry. Guarded access - src/utils/host.ts is new: getLogger, getCwd and inspectValue. Reading process.logger or process.cwd directly throws a ReferenceError in a browser, and inspect needs node's util module. - Thirteen bare `process.logger || console` sites and two `process.logger?.info(...)` sites now call getLogger(). getCwd replaces process.cwd() in getFileStream, and inspectValue replaces util.inspect in RequestClient, using util in node and JSON in a browser. Node-only imports - `import * as https from 'https'` was type-only in five files and is now `import type`, so tsc emits no require for them. The one value use - the https.Agent in RequestClient - is behind isNode() and requires the module at that point. - The `fs` import in types/WriteStream.ts is type-only for the same reason. - getFileStream no longer imports @sasjs/utils/file statically. That import dragged its csvFile module, and @fast-csv with it, and the stream shim @fast-csv needs, into the browser bundle. Both call sites are behind isNode(). The browser build - nodePolyfillPlugin is gone, along with the polyfill tree it bundled. - resolve.fallback covers the builtins the source still names inside node-guarded code (https, fs, readline, path), and @sasjs/utils/file is aliased to false rather than shimmed. - browser-shims/process.js is new: a twenty-line process stub. It is still needed, and the reason is concrete - @sasjs/utils/formatter/regexp.js reads process.platform unconditionally and the formatter runs in browsers to build request bodies, readable-stream reads process.nextTick, and util-deprecate reads process.env. versions stays empty so isNode() remains false in a browser. What it measures dist/sasjs-adapter.js 1.40 MB -> 0.96 MB dist/minified_sas9.js 436 kB -> 432 kB packed tarball 226 files / 739 kB -> 231 files / 419 kB Markers in the shipped JavaScript: before after process.binding shim 3 0 safe-buffer 1 0 os.homedir() 1 0 Node stdlib polyfill packages 0 0 (were bundled, now absent) new Function 2 2 process.platform / process.env 10 6 (reads, now against the stub) The compiled tree under the package root carries none of them. Verification - npm run build exits 0. - npm test: 52 suites, 347 tests, all passing. - release/pack-check.mjs passes. - API parity: the packed node entry exposes the same 25 named exports and 48 prototype methods as the 4.19.4 bundle, with no differences in either direction. - Browser load: the packed dist/sasjs-adapter.js runs in a VM context with no process global, and `new SASjs('http://localhost')` constructs with 47 instance methods. That is the closest local stand-in for a browser. - prettier passes on the touched files. Still open - `new Function` remains. It is webpack's runtime global helper, emitted whenever the bundle needs the global object; output.globalObject does not remove it, and removing it means a different browser bundler. - The unguarded `process.platform` in @sasjs/utils/formatter/regexp.js is the same class of bug this commit fixes here. Fixing it upstream in @sasjs/utils would be better than answering it with a stub, and would leave the stub answering only nextTick and env. This commit carries a build: subject, so it does not cut a release. A publish needs a releasing type (fix or feat) on the merge. Not published. The browser suites run against a real SAS server tomorrow, which is what confirms the refactor did not break a runtime path. * fix(deps): bump @sasjs/utils to the release that guards the platform read @sasjs/utils 3.7.3 guards the process.platform reads in newLine, isWindows and isLinux (sasjs/utils#281), which is the downstream half of this branch: the adapter's browser bundle only needed a Node stdlib polyfill tree because those reads threw in a browser. 3.7.1 -> 3.7.3, skipping 3.7.2. What that changes here: the only process.platform read left in the browser bundle is inside utils' own guard, so the stub no longer needs the field for correctness. It keeps it, and its comment now names the reads that remain - readable-stream's process.nextTick, util-deprecate's process.env and deprecation flags, and a bundled asap helper that falls back to nextTick when queueMicrotask is missing. Verification - npm run build exits 0; npm test passes 52 suites / 347 tests. - Packed: 231 files / 419 kB, carrying @sasjs/utils 3.7.3. - Markers in the shipped JavaScript are down to webpack's runtime global helper (`new Function`, once per browser bundle). No process.binding shim, no safe-buffer, no os.homedir, no Node stdlib polyfill packages, and none in the compiled tree under the package root. - The browser bundle still loads and constructs an instance in a VM context with no process global (47 instance methods). - Node entry parity is unchanged: 25 named exports and 48 prototype methods, matching the 4.19.4 bundle. CI reruns on this push, so the adapter-tests deployables for the browser run are rebuilt from this commit. * build(release): guard the browser bundle against the Node polyfill tree The browser build answers the Node property reads it still makes with browser-shims/process.js, and the polyfill tree it used to carry is gone. Two things put that tree on the bundle's path, and nothing checked either of them: - webpack aliasing the Node builtins (`crypto` and `constants` reach crypto-browserify), and - browser-reachable source importing the @sasjs/utils barrel, which re-exports ./fs, whose hash module imports crypto - the path sasjs/utils#275 describes. release/bundle-check.mjs fails on both. It reads the built bundles (dist/ after publishInit, build/dist/ before) and looks for code, not package names: webpack renumbers modules and rewrites requests, so a name proves nothing either way. The published 4.19.4 bundle carried 19 browserify-sign modules and not one occurrence of the string "browserify-sign", which is why #275's suggested grep cannot see it. The signatures used here are crypto-browserify's index comment, its createCredentials stub and constants table, browserify-sign's algorithm table, public-encrypt's OAEP function, the vm shim's runInThisContext, and the process/browser shim. safe-buffer and string_decoder are deliberately left out: a dependency bump can legitimately bring those back, and that is not this guard's concern. It also fails on a barrel or @sasjs/utils/fs import anywhere in src/ outside specs and fixtures - the same chain reached without the plugin's help. Wired into both workflows: after the build in build-unit-tests.yml, and after publishInit in npmpublish.yml so it checks the tree that ships. Verified - The guard passes on this branch: 2 bundles, 114 source files, exit 0. - It bites on the real pre-fix artifact: with the published 4.19.4 bundle in place of the built one, it names all eight signatures and exits 1. - It bites on the source rule: a barrel import in src/utils/convertToCsv.ts fails it at that file and line. - End to end, the regression it guards against: the polyfill plugin back in webpack.config.js plus a used barrel import rebuilds the bundle at 2767 kB (from 937 kB) carrying crypto-browserify, browserify-sign, public-encrypt and the vm shim, and the guard names all of it. The plugin alone no longer re-adds the suite, because nothing in the current graph requests those builtins - the barrel import is what does. - No bundle present: it exits 1 with "run npm run build first" rather than passing silently. - npm test: 52 suites / 347 tests pass. npm run lint passes. prettier clean under the repo's .prettierrc. * fix(host): guard the node-only paths with isNode, and keep util out of the bundle Three findings from the review of this branch, all confirmed against the built bundle before fixing. **The util npm polyfill was in the browser bundle.** host.inspectValue's `require('util')` is statically resolved, and the util package is in the tree, so webpack bundled 28 KiB of Node's own util source (util.js 19697 bytes plus support/types.js 8672 and support/isBufferBrowser.js 203) into both browser outputs, for a call a browser never makes. `resolve.fallback` does not fix this: webpack only consults the fallback when a request does not resolve, and here it resolves. It needs an alias, like @sasjs/utils/file already had. Measured: the browser bundle goes 938 kB -> 860 kB, and the util signatures go to zero. **The `typeof process === 'undefined'` guards were dead.** ProvidePlugin injects the process stub into every module - the bundle carries `/* provided dependency */ var process = __webpack_require__(5748)` in five modules - so `process` is always defined and the getLogger/getCwd guards never took their browser branch, while inspectValue took its node branch and called the polyfill. They now use isNode(), which reads process.versions.node, and the stub's empty `versions` object is what keeps it false in a browser. **deployZipFile reached for the filesystem unguarded.** Its `require('@sasjs/utils/file')` sat outside any isNode() guard, unlike the getFileStream call site, so in a browser - where the alias maps that path to nothing - it threw "createReadStream is not a function". It now refuses the way refreshTokensForViya does, with `Method 'deployZipFile' can only be used by Node.`, so webpack.config.js's claim that every call site is guarded holds. Also: the new util signature in release/bundle-check.mjs, so the guard covers this polyfill too, and the comments in webpack.config.js and host.ts now say what the build actually does. Verified - npm test: 53 suites / 355 tests pass. Two new specs cover the fixed paths: src/utils/spec/host.spec.ts for both environments, and a browser case for deployZipFile. - Each new test bites for the reason it names: reverting inspectValue to the typeof-process guard fails the JSON fallback case (util.inspect output instead), and removing the deployZipFile guard fails its case (no rejection). - node release/bundle-check.mjs passes with the util signature in place; before the alias it failed naming inspect.styles in both bundles. - npm run lint passes; prettier clean on the changed files. * fix(deps): bump @sasjs/utils to 4.0.0, the release that drops fs from the barrel @sasjs/utils 4.0.0 stops re-exporting './fs' from its barrel (sasjs/utils#283, closing sasjs/utils#275). './fs' is the only module there that imports crypto, and the re-export put the AES/RSA/ECDSA polyfill suite - plus a vm shim's dynamic code execution - into every browser bundle that imported anything from the barrel. Nothing in this package changes with it: the adapter imports the deep subpaths and never the barrel, which release/bundle-check.mjs asserts on every build. So this is the pin moving to the release the estate is on, not a migration. 3.7.3 -> 4.0.0. Verified - npm run build exits 0; npm test passes 53 suites / 355 tests. - release/bundle-check.mjs passes: no polyfill tree, no barrel import. - npm audit --omit=dev --audit-level=low: 0 vulnerabilities. - npm run lint passes. - Bundle sizes unchanged: dist/sasjs-adapter.js 880794 bytes, dist/minified_sas9.js 352378. * fix(deps): drop the devDependencies the rewritten browser build no longer uses node-polyfill-webpack-plugin, process and terser-webpack-plugin lost their last references on this branch: webpack.config.js no longer requires the polyfill plugin, ProvidePlugin points at browser-shims/process.js rather than process/browser, and minimize is false with no minimizer, so terser is never invoked. A repo-wide grep finds no other reference to any of the three. The lockfile drops 1406 lines. `process` remains in the tree as a transitive dependency of cypress, which is the one entry that stays. Verified - npm run build exits 0, and both bundles are byte-identical to before the removal: dist/sasjs-adapter.js 880794 bytes, dist/minified_sas9.js 352378. - npm test: 53 suites / 355 tests pass. release/bundle-check.mjs passes. - npm audit --omit=dev --audit-level=low: 0 vulnerabilities. npm run lint passes. * fix(package): point the example at the current major's CDN entry example.html pinned npm/@sasjs/adapter@4 while the README's download link moves to @5, so anyone copying the example got the previous major's pre-dist bundle layout. jsDelivr resolves a package's default file from the `browser` field before `main` - verified against form-data@4.0.6, whose browser field resolves to /lib/browser.min.js while its main is ./lib/form_data - so with the browser field this branch adds, npm/@sasjs/adapter@5 resolves to dist/sasjs-adapter.min.js. That is the file the README names. * fix(package): name the browser file in the example's combine URL The previous commit moved example.html from @4 to @5 but left the combine URL resolving the package default. That was a guess I should not have made: the combine endpoint does not resolve a `browser` field the way a plain npm URL does. Measured today: - plain npm URL: jsDelivr resolves the default file from `browser` before `main` - form-data@4.0.6 has main ./lib/form_data and browser ./lib/browser, and its entrypoint resolves to /lib/browser.min.js. - combine: for that same package it fails, "Couldn't find the requested file /lib/browser in form-data" - it takes the field as a literal path and does not add the extension. The current @4 URL works only because @4 has no browser field, so combine falls through to main. So the combine URL now names the file, which is also the file the README's download link names: npm/@sasjs/adapter@5/dist/sasjs-adapter.min.js The explicit-path form is proven: combine/npm/jquery@3.5.1,npm/form-data@4.0.6/lib/browser.min.js returns 200 and reports both files in its combined header. * fix(package): ship DISCLOSURE, and make pack-check require it The rewritten `files` allowlist dropped the DISCLOSURE entry, and an allowlist only ships what it lists: a pack simulation of the post-publishInit tree reported 231 files with LICENSE and README.md present and DISCLOSURE absent, while package:lib copies it into build/ and npmpublish.yml stages the npm release on the basis that it sits at the root of the tarball. So the dual-use declaration was present at publish time and silently filtered out of it. DISCLOSURE is back in the allowlist - the simulation now reports 232 files with DISCLOSURE present. release/pack-check.mjs gains the other half of the rule: a REQUIRED list (DISCLOSURE, LICENSE, README.md, package.json) that fails the build when any of them is missing from the packed file list, reported with the reason each one matters. The existing rule catches an allowlist reaching too far; this one catches it reaching too little, which is the same class of mistake and shipped the same way - past CI, because nothing looked at the packed file list. Verified - Pack simulation: 231 files before, 232 after, DISCLOSURE present, tarball 381796 bytes. - The new rule bites: with "DISCLOSURE" removed from the allowlist, pack-check exits 1 naming it and the reason; restored, it exits 0. - npm run build, npm test (53 suites / 355 tests), release/bundle-check.mjs, release/pack-check.mjs and npm run lint all pass; prettier clean. * fix(tests): guard the adapter-tests services against an empty sasjs_tables The sendArr and sendObj services test `%symexist(sasjs_tables)` before looping, but the adapter always sends that form field - formatDataForRequest ends with `result['sasjs_tables'] = sasjsTables.join(' ')`, which is '' when the payload carries no tables. So the variable exists and is blank, the %symexist branch is taken, and `%sysfunc(countw(&sasjs_tables))` is called with no argument: ERROR: The function COUNTW referenced by the %SYSFUNC or %QSYSFUNC macro function has too few arguments. ERROR: The %TO value of the %DO I loop is invalid. ERROR: The macro X will stop executing. Live on Viya (nextviya, Compute Reusable), the two `runAsTask behaviour / no inputs` tests failed with debug on because JES then answers with the job-error page and the adapter reports "Unable to find webout content". With debug off the same job error is invisible: the adapter resolved and handed the caller the string "Job Error\nA procedure in the SAS program ...", which the test's `res?.ok === true` assertion accepted. The services now test for length as well as existence, which is the same condition core's own generated services use (`%length(&sasjs_tables.X)>1` in viya/mv_createwebservice.sas). The README recipe carries the same fix, since that is the version users copy into their own apps. Verified on nextviya against adapter 5.0.4 from this branch, appLoc /Users/viyademo18/adapter-tests: 53/53 pass with debug on (was 50/53) and 53/53 with debug off, and the no-inputs response is the webout payload again rather than the job-error text. * fix(ci): let the deployables appLoc be configured, not hardcoded The appLoc a streamed app is built against is baked into the artefact, so a reviewer who deploys it needs a path their own account can write to. The workflow hardcoded /Public/app/adapter-tests/pr-<n>, and on Viya /Public is not writable for a user-scoped account - the deploy fails with a 404 on the parent folder. That is what happened running this PR's own deployables recipe against nextviya, where the app had to be rebuilt locally for /Users/viyademo18. The base now comes from, in order: the `apploc` input of a manual run, the ADAPTER_TESTS_APPLOC repository variable, then the previous default. The /pr-<number> suffix is still appended by the workflow, so two PRs cannot collide and a redeploy does not hit the "already exists" conflict. The PR comment says how to change it. * fix(ci): make the pack-check REQUIRED list only what the allowlist can break Three of the four REQUIRED entries could not fail. npm puts package.json, README.md and LICENSE in every tarball whatever `files` says, so `!files.includes(name)` on them asserted nothing about the allowlist: a future edit that dropped README.md or LICENSE from `files` would still pack 232 files and still pass this check. Proved by mutation on the post-publishInit tree: dropping DISCLOSURE from the allowlist packs 231 files and fails the check naming it, while dropping README.md and LICENSE packs the same 232 files and passes. The three are now documented as npm's own behaviour rather than checked, so the list reads as exactly what the allowlist can break - which today is the one entry that was broken. --------- Co-authored-by: sasjs-dev[bot] <sasjs-dev[bot]@users.noreply.github.com>
sasjs-gha Bot
pushed a commit
to sasjs/adapter
that referenced
this pull request
Oct 16, 2026
# [5.1.0](v5.0.4...v5.1.0) (2026-10-16) ### Bug Fixes * **release:** read the pack report in the shape npm 12 reports it ([#933](#933)) ([6ff349c](6ff349c)), closes [sasjs/cli#1483](sasjs/cli#1483) ### Features * **package:** ship the adapter as compiled modules, with the browser build in dist ([#932](#932)) ([187f50e](187f50e)), closes [sasjs/utils#281](sasjs/utils#281) [sasjs/utils#275](sasjs/utils#275) [sasjs/utils#283](sasjs/utils#283) [sasjs/utils#275](sasjs/utils#275)
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
1 participant
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.
newLine(),isWindows()andisLinux()readprocess.platformdirectly. Fine in Node, aReferenceErrorin a browser - and the browser case is not hypothetical:newLineis part of the formatter, which runs in browsers to build request bodies.Where this came from
Found downstream while removing the Node polyfill tree from the adapter's browser bundle. That bundle has been carrying shimmed crypto, stream, os, path and buffer, and one of the few things it actually needed them for was answering reads like this. With these three functions guarded, the adapter's browser bundle needs a twenty-line process stub instead of thousands of lines of bundled Node internals.
Changes
newLine()returns the unix ending where there is no process.isWindows()andisLinux()answer false there. Both are exported from the package root and callable from browser code.newLineis resolved by keeping the formatter self-contained rather than importingisWindows: that module importspathand is loaded from the root, which is what pulled Logger in and brokecli.spec.ts. The comment now records that reason.Tests
The failure mode is invisible in Node, so each spec deletes the process global and asserts the fallback, then restores it. The existing specs mutated
process.platformwithout restoring, so they are pinned down the same way.Proven by reintroducing the bug: the new case fails with
ReferenceError: process is not defined, and passes with the guard in place.Full suite: 40 suites, 445 tests passing. Prettier clean on
src/**/*.ts.