Skip to content

feat!: ESM + Node 18#643

Merged
mshanemc merged 33 commits into
mainfrom
mdonnalley/esm
Oct 12, 2023
Merged

feat!: ESM + Node 18#643
mshanemc merged 33 commits into
mainfrom
mdonnalley/esm

Conversation

@mdonnalley

@mdonnalley mdonnalley commented Oct 6, 2023

Copy link
Copy Markdown
Contributor
  • Migrate to ESM
  • Node 18 minimum
  • Repo clean up (prettier, linting, etc...)
  • Adds integration test

@W-14259714@

Closes #610

@git2gus

git2gus Bot commented Oct 9, 2023

Copy link
Copy Markdown

This issue has been linked to a new work item: W-14259714

Comment thread package.json
"@oclif/core": "^2.11.8",
"chalk": "^4",
"@oclif/core": "^3.0.3",
"chalk": "^5",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

any interesting learnings from the chalk/inquirer changes?

Comment thread src/hooks/init.ts Outdated
const clientRoot = this.config.scopedEnvVar('OCLIF_CLIENT_HOME') || path.join(this.config.dataDir, 'client')

const {config, error: throwError} = this
const binPath = config.binPath || config.bin

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can this go into the eslint rules.

Suggested change
const binPath = config.binPath || config.bin
const binPath = config.binPath ?? config.bin

Comment thread src/hooks/init.ts Outdated
} catch (error: any) {
if (error.code !== 'ENOENT') ux.error(error.stack)
if (error.code !== 'ENOENT') throwError(error.stack)
if ((global as any).testing) return false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

where's the global and how is it being set? And is there a cleaner way to do this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll just remove it - I'm assuming it was added at some point for testing but I don't see anything setting it anymore

Comment thread src/tar.ts
const debug = require('debug')('oclif-update')
import {touch} from './util.js'
const debug = makeDebug('oclif-update')
import crypto from 'node:crypto'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

here's an example of a great place to defer via dynamic imports so that we only load crypto when it's needed (in extract when there is a sha

Comment thread src/update.ts Outdated
constructor(private config: Config) {
this.clientRoot = config.scopedEnvVar('OCLIF_CLIENT_HOME') || path.join(config.dataDir, 'client')
this.clientBin = path.join(this.clientRoot, 'bin', config.windows ? `${config.bin}.cmd` : config.bin)
this.clientRoot = config.scopedEnvVar('OCLIF_CLIENT_HOME') || join(config.dataDir, 'client')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
this.clientRoot = config.scopedEnvVar('OCLIF_CLIENT_HOME') || join(config.dataDir, 'client')
this.clientRoot = config.scopedEnvVar('OCLIF_CLIENT_HOME') ?? join(config.dataDir, 'client')

Comment thread src/update.ts Outdated
Comment on lines +43 to +47
if (typeof body === 'string') {
return JSON.parse(body)
}

return body

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (typeof body === 'string') {
return JSON.parse(body)
}
return body
return typeof body === 'string' ? JSON.parse(body) : body

Comment thread src/update.ts
throw error
}
if (windows) {
const body = `@echo off

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tidyness: put the shell scripts in separate files that export the value, and then import that?

Comment thread test/integration/sf.integration.ts Outdated
let stableVersion: string
let sf: string

const versionToUpdateTo = '2.12.7-esm.0'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we'll have to be careful to keep this around forever?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll update it to always grab the version just before the current nightly, which will likely always exist

Comment thread test/integration/sf.integration.ts Outdated
Comment on lines +77 to +80
const tarball =
process.platform === 'win32'
? `https://developer.salesforce.com/media/salesforce-cli/sf/channels/${channel}/sf-win32-x64.tar.gz`
: `https://developer.salesforce.com/media/salesforce-cli/sf/channels/${channel}/sf-linux-x64.tar.gz`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const tarball =
process.platform === 'win32'
? `https://developer.salesforce.com/media/salesforce-cli/sf/channels/${channel}/sf-win32-x64.tar.gz`
: `https://developer.salesforce.com/media/salesforce-cli/sf/channels/${channel}/sf-linux-x64.tar.gz`
const platform = process.platform === 'win32' ? 'win32' : 'linux'
const tarball = `https://developer.salesforce.com/media/salesforce-cli/sf/channels/${channel}/sf-${platform}-x64.tar.gz`

Comment thread test/integration/sf.integration.ts Outdated
console.log('Success!')

console.log('Linking plugin-update...')
// Running `plugins link` is very slow on github-action's windows runners. Writing this file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

will you change this since oclif/plugin-plugins#679 merged?

@mshanemc

mshanemc commented Oct 10, 2023

Copy link
Copy Markdown
Member

QA notes:

using @salesforce/cli/2.13.6 darwin-x64 node-v18.15.0 which is ESM, plus this plugin via plugins:link

❌ every command warns about the linked plugin. Seems fine, unless you run a command with --json at which point it's still showing in the terminal output instead of in json result.warnings.

 ›   Warning: @oclif/plugin-update is a linked ESM module and cannot be auto-transpiled. Existing compiled source will 
 ›   be used instead.

sf update (no-op), then run some commands with that version

  • sf org list
  • sf org open -o [org]
  • sf data query -t -q "SELECT MemberType, MemberName, IsNameObsolete, RevisionCounter FROM SourceMember" -o[org]
  • sf data query -t -q "SELECT MemberType, MemberName, IsNameObsolete, RevisionCounter FROM SourceMember" -o[org] --json [this is where I found the json problem for linked esm plugins]

sf update stable => @salesforce/cli: Updating CLI from 2.13.6-9ea464d to 2.11.8-c9872b8
Should I interpret this as, "you can't update the CLI below the ESM line?"

(node:22163) [ERR_UNKNOWN_FILE_EXTENSION] TypeError Plugin: @oclif/plugin-update [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /Users/shane.mclaughlin/eng/oclif-repos/plugin-update/src/commands/update.ts
module: @oclif/core@2.15.0
task: toCached
plugin: @oclif/plugin-update
root: /Users/shane.mclaughlin/eng/oclif-repos/plugin-update
See more details with DEBUG=*
(Use `node --trace-warnings ...` to show where the warning was created)

✅ release notes happen as expected
sf org list works, but with the same ERR_UNKNOWN_FILE_EXTENSION in the terminal output (it goes to stderr)

❌ now the user can't update back across the line to the esm version (because the update command isn't available)
➜ plugin-update git:(sm/qa-kit-esm) ✗ sf update nightly
(node:22764) [ERR_UNKNOWN_FILE_EXTENSION] TypeError Plugin: @oclif/plugin-update [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /Users/shane.mclaughlin/eng/oclif-repos/plugin-update/src/commands/update.ts
module: @oclif/core@2.15.0
task: toCached
plugin: @oclif/plugin-update
root: /Users/shane.mclaughlin/eng/oclif-repos/plugin-update
See more details with DEBUG=*
(Use node --trace-warnings ... to show where the warning was created)
› Warning: update nightly is not a sf command.
Did you mean data query? [y/n]: ^C


got back to a good state by uninstalling the linked plugin.

@mdonnalley

Copy link
Copy Markdown
Contributor Author

❌ every command warns about the linked plugin. Seems fine, unless you run a command with --json at which point it's still showing in the terminal output instead of in json result.warnings.

Confirmed that warning goes to stderr:

~/repos/oclif/plugin-update (mdonnalley/esm*) » sf version
 ›   Warning: @oclif/plugin-update is a linked ESM module and cannot be auto-transpiled. Existing compiled source will be used
 ›   instead.
@salesforce/cli/2.12.7-esm.0 darwin-arm64 node-v18.15.0

// silence stderr
~/repos/oclif/plugin-update (mdonnalley/esm*) » sf version 2> /dev/null
@salesforce/cli/2.12.7-esm.0 darwin-arm64 node-v18.15.0

// silence stdout
~/repos/oclif/plugin-update (mdonnalley/esm*) » sf version 1> /dev/null
 ›   Warning: @oclif/plugin-update is a linked ESM module and cannot be auto-transpiled. Existing compiled source will be used
 ›   instead.

mshanemc
mshanemc previously approved these changes Oct 11, 2023
@mshanemc

Copy link
Copy Markdown
Member

since linked plugins are sketchy, QA round 2 will use an installed qa release `3.2.4-qa.0

sf update nightly

@salesforce/cli: Updating CLI from 2.13.8-01c9e90 to 2.14.0-6c406ae (nightly)... ⣾ 58.3 MB/58.8 MB

sf update stable (down below ESM line) 2.14.0-6c406ae to 2.12.9-007e29c
✅ works. shows RN as expected

sf update available does what you'd expect. That list is getting pretty large

downgrade a long way via --interactive

? Select a version to update to 2.2.0
@salesforce/cli: Updating CLI from 2.12.9-007e29c to 2.2.0-4aecedc... ⡿ 20.1 MB/63.1 MB

the old org list table is so ugly. Let's get back to nightly

✅ looks great. 🚢

@mshanemc mshanemc merged commit be87bf6 into main Oct 12, 2023
@mshanemc mshanemc deleted the mdonnalley/esm branch October 12, 2023 14:48
khaled4vokalz added a commit to khaled4vokalz/plugin-update that referenced this pull request Jun 30, 2026
Installed CLIs live in directories named "<version>-<sha>" (e.g.
"1.2.3-abc1234"), but `this.config.version` is plain semver ("1.2.3").
Two guards meant to stop the running CLI from deleting itself both
failed for sha-suffixed installs:

- `tidy()`'s `isNotSpecial` used an exact-match `includes(version)`
  check, so "1.2.3-abc1234" was never recognized as the active version
  and lost its name-based protection. The `startsWith` guard that
  originally handled this (ca57247, oclif#621) was dropped in the ESM
  refactor (oclif#643); oclif#1290 later restored only the bin/current protection
  via the arg-swap fix, not the startsWith semantics.

- `touch()` joined the bare `config.version`, producing a path that
  does not exist on disk, so it silently no-op'd and never refreshed
  the active dir's mtime.

With both guards failing, once a CLI stayed on the latest version for
42+ days, `tidy()` deleted the directory the `current` symlink points
to, leaving `current` dangling and the CLI broken.

Restore the `startsWith` guard in `isNotSpecial` and resolve the real
"<version>-<sha>" directory in `touch()` via a shared `resolveActiveDir`
helper. Add regression tests covering both the tidy guard and the touch
backstop for sha-suffixed installs.

fixes: oclif#1361
khaled4vokalz added a commit to khaled4vokalz/plugin-update that referenced this pull request Jun 30, 2026
Installed CLIs live in directories named "<version>-<sha>" (e.g.
"1.2.3-abc1234"), but `this.config.version` is plain semver ("1.2.3").
Two guards meant to stop the running CLI from deleting itself both
failed for sha-suffixed installs:

- `tidy()`'s `isNotSpecial` used an exact-match `includes(version)`
  check, so "1.2.3-abc1234" was never recognized as the active version
  and lost its name-based protection. The `startsWith` guard that
  originally handled this (ca57247, oclif#621) was dropped in the ESM
  refactor (oclif#643); oclif#1290 later restored only the bin/current protection
  via the arg-swap fix, not the startsWith semantics.

- `touch()` joined the bare `config.version`, producing a path that
  does not exist on disk, so it silently no-op'd and never refreshed
  the active dir's mtime.

With both guards failing, once a CLI stayed on the latest version for
42+ days, `tidy()` deleted the directory the `current` symlink points
to, leaving `current` dangling and the CLI broken.

Restore the `startsWith` guard in `isNotSpecial` and resolve the real
"<version>-<sha>" directory in `touch()` via a shared `resolveActiveDir`
helper. Add regression tests covering both the tidy guard and the touch
backstop for sha-suffixed installs.

fixes: oclif#1361
jfeingold35 pushed a commit that referenced this pull request Jun 30, 2026
Installed CLIs live in directories named "<version>-<sha>" (e.g.
"1.2.3-abc1234"), but `this.config.version` is plain semver ("1.2.3").
Two guards meant to stop the running CLI from deleting itself both
failed for sha-suffixed installs:

- `tidy()`'s `isNotSpecial` used an exact-match `includes(version)`
  check, so "1.2.3-abc1234" was never recognized as the active version
  and lost its name-based protection. The `startsWith` guard that
  originally handled this (ca57247, #621) was dropped in the ESM
  refactor (#643); #1290 later restored only the bin/current protection
  via the arg-swap fix, not the startsWith semantics.

- `touch()` joined the bare `config.version`, producing a path that
  does not exist on disk, so it silently no-op'd and never refreshed
  the active dir's mtime.

With both guards failing, once a CLI stayed on the latest version for
42+ days, `tidy()` deleted the directory the `current` symlink points
to, leaving `current` dangling and the CLI broken.

Restore the `startsWith` guard in `isNotSpecial` and resolve the real
"<version>-<sha>" directory in `touch()` via a shared `resolveActiveDir`
helper. Add regression tests covering both the tidy guard and the touch
backstop for sha-suffixed installs.

fixes: #1361
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