feat!: migrate to ESM #437

mdonnalley · 2023-10-13T17:09:04Z

BREAKING CHANGES: ESM and node 18 minimum

Adds frequency and frequencyUnit settings to make warning frequency configurable
- Fixes Display warning less frequently #21
- Depends on fix: add frequency and frequencyUnit to warn-if-update core#827
Adds <CLI>_SKIP_NEW_VERSION_CHECK environment variable to disable version check
- Fixes Ability to conditionally disable? #279
- Closes Allow users to disable update checks #358 (Thank you @RealOrangeOne)

Closes #400

Release Notes

Added these env vars:
<CLI>_SKIP_NEW_VERSION_CHECK: Skip the version update check
<CLI>_NEW_VERSION_CHECK_FREQ: the frequency at which the version warning should be shown
<CLI>_NEW_VERSION_CHECK_FREQ_UNIT: the unit of time used to set the frequency. Defaults to minutes (can be days, hours, minutes, seconds, milliseconds)

BREAKING CHANGES: ESM and node 18 minimum

git2gus · 2023-10-13T17:09:31Z

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

RealOrangeOne · 2023-10-13T20:30:21Z

This seems to contain my PR, but without reference or attribution. Could you cherry-pick the commit instead?

Fixes #279 This can speed up execution, reduce unnecessary network requests, and clean up log files.

mshanemc · 2023-10-16T17:31:56Z

src/hooks/init/check-update.ts

+const readJSON = async <T>(file: string): Promise<{contents: T; stat: Stats}> => ({
+  contents: JSON.parse(await readFile(file, 'utf8')),
+  stat: await stat(file),
+})


perf: promise.all the 2 tasks, then return them in the object.

[small perf nit, but this runs before every command]

mshanemc · 2023-10-16T17:36:02Z

src/hooks/init/check-update.ts

-    authorization = '',
-  } = (config.pjson.oclif as any)['warn-if-update-available'] || {}
+    timeoutInDays = 60,
+  } = config.pjson.oclif['warn-if-update-available'] || {}


Suggested change

} = config.pjson.oclif['warn-if-update-available'] || {}

} = config.pjson.oclif['warn-if-update-available'] ?? {}

mshanemc · 2023-10-16T17:41:37Z

src/hooks/init/check-update.ts

+    timeoutInDays = 60,
+  } = config.pjson.oclif['warn-if-update-available'] || {}
+
+  const warningFrequency = (config.scopedEnvVar('NEW_VERSION_CHECK_FREQ') as number | undefined) ?? frequency


TS lets you make ASsertions about what's in somebody's env (lying to the compiler), but that doesn't do any checks or conversions in the real world.

example: if their frequency was ham and the unit was sandwiches then convertToMs is going to have unhandled errors.

Also, envs would be a string anyway, so you'll need to parseInt warningFrequency, right?

let's have some tests for handling of these envs.

mshanemc · 2023-10-16T17:44:26Z

src/hooks/init/check-update.ts

+
+      // If the file was modified before the timeout, don't show the warning
+      if (
+        warningFrequency &&


confirming that if warningFrequency is set to 0 you'd short-circuit the rest of the conditional?

Yes, I think that makes sense. Frequency of 0 causes the default behavior of seeing the warning on every command execution.

I guess the alternative is that a frequency of 0 turns the warning off and you never see it again? We already have an env var for that so I don't see a reason to duplicate it

mshanemc · 2023-10-16T17:46:33Z

src/hooks/init/check-update.ts

+      const {gt} = await import('semver')
+      if (distTags[tag] && gt(distTags[tag].split('-')[0], config.version.split('-')[0])) {


nit: since you've already got distTags, you could check that and avoid the semver import if it weren't needed

mshanemc · 2023-10-16T17:48:26Z

src/hooks/init/check-update.ts

+
+        // Update the modified time (mtime) of the version file so that we can track the last time we
+        // showed the warning. This makes it possible to respect the frequency and frequencyUnit options.
+        await utimes(file, new Date(), new Date())


does the utimes op to wait on the lodash import and warning to happen first? or could it be run in parallel with the import?

mshanemc · 2023-10-16T17:52:45Z

src/get-version.ts

  ].join('/')
  const headers = authorization ? {authorization} : {}
  await mkdir(dirname(file), {recursive: true})
  await writeFile(file, JSON.stringify({current: version, headers})) // touch file with current version to prevent multiple updates
-  const {body} = await HTTP.get<any>(url, {headers, timeout: 5000})
-  await writeFile(file, JSON.stringify({...body['dist-tags'], current: version, authorization}))
+  const {body} = await HTTP.get<{'dist-tags': string[]}>(url, {headers, timeout: 5000})


does the http.get need to wait on the mkdir/writeFile?

I'm not sure - and I don't want to take the risk of changing it, especially since this script is run in a unref'd spawned process. So any performance improvements in there will never be noticed by anyone

mshanemc · 2023-10-16T20:41:25Z

src/hooks/init/check-update.ts

+}
+
+export function semverGreaterThan(a: string, b: string): boolean {
+  return a.localeCompare(b, undefined, {numeric: true, sensitivity: 'base'}) > 0


cool. I saw the semver removal and wondered what was going to happen here.

mshanemc · 2023-10-17T14:59:02Z

QA Notes: created 2.1.2-qa-0 to tet with.

switched to 2.3.8 via udpate --interactive
ran org list.
❌ › Warning: @salesforce/cli update available from 2.3.8 to 2.5.8..
Why prompt me to update to 2.5.8?

ok, let's just go with and update to 2.5.8 to see if it knows about updates beyond that.

ran org list a few times, not update suggestions.
did some debugging...mtime on the cache file is today, so it's not "stale" until Dec 16 (60 days). But the file is already 7 weeks behind.
[I think this is because I usually run things on nightly channel so my file doesn't get updated often? but somehow the mtime is updated?]

deletes the version file, run command again.
✅ version file is now correct
✅ gives correct warning

testing envs:
SF_SKIP_NEW_VERSION_CHECK=true
✅ file is not created

SF_NEW_VERSION_CHECK_FREQ=30 SF_NEW_VERSION_CHECK_FREQ_UNIT=seconds sf org list
✅ running repeatedly seems to display after > 30 seconds.

🔴 potential design flaw:

use of the file mtime to mark both the most recent warning emitted AND the file staleness.
ex: you have you warn freq set to daily. each day, you get a warn and then the file mtime is set.
File never becomes stale? So you update to an old version instead of current

mshanemc · 2023-10-17T18:18:13Z

QA notes (2.1.2-qa.1)

update to 2.8.3
✅ gets warning when RN command is called Warning: @salesforce/cli update available from 2.8.3 to 2.12.9
last-warning file is created

delete file
✅ running a command recreates last-warning

delete both files
✅ recreated files, see warning after a few commands

SF_NEW_VERSION_CHECK_FREQ=30 SF_NEW_VERSION_CHECK_FREQ_UNIT=seconds sf org list
✅ works as expected

set latest to 2.0.0
✅ no warnings

set latest to 2.8.3 (the current version)
✅ no warnings

set latest to 2.8.4 (the very next version)
✅ warning

SF_NEW_VERSION_CHECK_TAG=nigtly SF_FORCE_VERSION_CACHE_UPDATE=true sf org list
✅ updates file

SF_NEW_VERSION_CHECK_TAG=nigtly sf org list (bad tag)
✅ no warnings, no errors

SF_NEW_VERSION_CHECK_TAG=nightly sf org list
✅ shows correct nightly update available

feat!: migrate to ESM

b1935ce

BREAKING CHANGES: ESM and node 18 minimum

mdonnalley added the enhancement label Oct 13, 2023

mdonnalley added 3 commits October 13, 2023 11:17

feat: add SKIP_UPDATE_CHECK env var (#358)

1b815a8

chore: bump @oclif/core

23a5883

feat: make warning frequency configurable

89d3e57

feat: allow users to disable update checks

3cc0a20

Fixes #279 This can speed up execution, reduce unnecessary network requests, and clean up log files.

salesforce-cla bot added the cla:signed label Oct 13, 2023

mshanemc requested changes Oct 16, 2023

View reviewed changes

mdonnalley added 8 commits October 16, 2023 13:56

chore: code review

ffc4e1c

test: add real tests

139d7a2

chore: remove rogue console.log

f837332

fix: use scoped env var

f02dde6

Merge branch 'main' into mdonnalley/esm

b71db23

chore: bump oclif/core

d1c31de

chore: update dev.cmd

9b83706

chore: more code review

bb5ddbb

mshanemc approved these changes Oct 16, 2023

View reviewed changes

chore(release): 2.1.2-qa.0 [skip ci]

d1a4de1

mdonnalley and others added 2 commits October 17, 2023 10:41

fix: use separate file for tracking last warning

c561e9e

chore(release): 2.1.2-qa.1 [skip ci]

229b551

mshanemc merged commit 15e81f5 into main Oct 17, 2023

mshanemc deleted the mdonnalley/esm branch October 17, 2023 18:25

w-andre mentioned this pull request Dec 4, 2023

Documentation regarding environment variable SF_SKIP_VERSION_CHECK incorrect forcedotcom/cli#2590

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: migrate to ESM #437

feat!: migrate to ESM #437

mdonnalley commented Oct 13, 2023 •

edited

Loading

git2gus bot commented Oct 13, 2023

RealOrangeOne commented Oct 13, 2023

mshanemc Oct 16, 2023

mshanemc Oct 16, 2023

mshanemc Oct 16, 2023

mshanemc Oct 16, 2023

mshanemc Oct 16, 2023

mdonnalley Oct 16, 2023

mshanemc Oct 16, 2023

mshanemc Oct 16, 2023

mshanemc Oct 16, 2023

mdonnalley Oct 16, 2023

mshanemc Oct 16, 2023

mshanemc commented Oct 17, 2023

mshanemc commented Oct 17, 2023

	} = config.pjson.oclif['warn-if-update-available'] \|\| {}
	} = config.pjson.oclif['warn-if-update-available'] ?? {}

		const {gt} = await import('semver')
		if (distTags[tag] && gt(distTags[tag].split('-')[0], config.version.split('-')[0])) {

feat!: migrate to ESM #437

feat!: migrate to ESM #437

Conversation

mdonnalley commented Oct 13, 2023 • edited Loading

Release Notes

git2gus bot commented Oct 13, 2023

RealOrangeOne commented Oct 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshanemc commented Oct 17, 2023

mshanemc commented Oct 17, 2023

mdonnalley commented Oct 13, 2023 •

edited

Loading