-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: migrate to ESM #683
Conversation
BREAKING CHANGES: ESM and node 18 minimum
This issue has been linked to a new work item: W-14314433 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice integration tests
.github/workflows/automerge.yml
Outdated
@@ -2,9 +2,12 @@ name: automerge | |||
on: | |||
workflow_dispatch: | |||
schedule: | |||
- cron: '17 2,5,8,11 * * *' | |||
- cron: '42 2,5,8,11 * * *' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oclif might not have the same concurrent job limits as Salesforce so maybe we should spread these around a bit?
- cron: '42 2,5,8,11 * * *' | |
- cron: '12 2,5,8,11 * * *' |
.github/workflows/test.yml
Outdated
@@ -22,12 +22,26 @@ jobs: | |||
retry_wait_seconds: 60 | |||
command: npm install -g @salesforce/cli@$nightly --omit=dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does the $
do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it does anything - it probably was a typo and we got lucky that npm doesn't care???
src/commands/plugins/inspect.ts
Outdated
|
||
function trimUntil(fsPath: string, part: string): string { | ||
const parts = fsPath.split(path.sep) | ||
const indices = parts.reduce((a, e, i) => (e === part) ? a.concat([i]) : a, [] as number[]) | ||
// eslint-disable-next-line unicorn/no-array-reduce | ||
const indices = parts.reduce((a, e, i) => (e === part ? [...a, i] : a), [] as number[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduce in TS accepts a type like parts.reduce<number[]> to avoid the as
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what a,e,i represent
src/commands/plugins/inspect.ts
Outdated
const dependencyPath = path.join(...dependency.split('/')) | ||
let start = path.join(plugin.root, 'node_modules') | ||
const paths = [start] | ||
while ((start.match(/node_modules/g) || []).length > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while ((start.match(/node_modules/g) || []).length > 1) { | |
while ((start.match(/node_modules/g) ?? []).length > 1) { |
src/commands/plugins/inspect.ts
Outdated
paths.push(start) | ||
async parsePluginName(input: string): Promise<string> { | ||
if (input.includes('@') && input.includes('/')) { | ||
input = input.slice(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's avoid mutating the input value to avoid future contributors experiencing unexpected side effects with name
src/hooks/update.ts
Outdated
} catch (error: any) { | ||
this.error(error.message) | ||
} catch (error: unknown) { | ||
const {message} = error as {message: string} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this trusts that nothing down the stack throws anything that doesn't match the js Error class.
conditional would be better (ex: if (error instanceof Error)
or if ('message' in error)
etc.
this.error doesn't handle undefineds (according to its types)
src/util.ts
Outdated
function compare(a: sortBy.Types | sortBy.Types[], b: sortBy.Types | sortBy.Types[]): number { | ||
a = a === undefined ? 0 : a | ||
b = b === undefined ? 0 : b | ||
type Types = boolean | number | string | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming: are they CompareTypes? SortByTypes? ComparableSortableTypes?
src/util.ts
Outdated
return compare(a.slice(1), b.slice(1)) | ||
} | ||
function compare(a: Types | Types[], b: Types | Types[]): number { | ||
a = a === undefined ? 0 : a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would mutate the values outside of this function.
you probably want a new const for them
src/yarn.ts
Outdated
// Fix windows bug with node-gyp hanging for input forever | ||
// if (this.config.windows) { | ||
// forked.stdin.write('\n') | ||
// } | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still needed?
describe('basic', () => { | ||
it('should return "No Plugins" if no plugins are installed', async () => { | ||
await PluginsIndex.run([], process.cwd()) | ||
expect(stubs.stdout.firstCall.firstArg).to.equal('No plugins installed.\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these can be brittle when something else hits stdout (warnings about updates, etc).
Maybe flatmap the callsArgs, and assert the presence of the message you expect in the array?
src/plugins.ts
Outdated
if (!match) return name | ||
return match[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!match) return name | |
return match[1] | |
return match?.[1] ?? name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few TS suggestions
src/plugins.ts
Outdated
const list = await this.list() | ||
const friendly = list.find((p) => this.friendlyName(p.name) === this.friendlyName(name)) | ||
const unfriendly = list.find((p) => this.unfriendlyName(p.name) === this.unfriendlyName(name)) | ||
const link = list.find((p) => p.type === 'link' && path.resolve(p.root) === path.resolve(name)) | ||
const link = list.find((p) => p.type === 'link' && resolve(p.root) === resolve(name)) | ||
return (friendly ?? unfriendly ?? link ?? false) as | ||
| Interfaces.PJSON.PluginTypes.Link | ||
| Interfaces.PJSON.User |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be Interfaces.PJSON.PluginTypes.User
instead of Interfaces.PJSON.User
? That's what this.list
returns?
that change seems to build and test OK, then the ASsertions aren't necessary.
src/util.ts
Outdated
|
||
public add(...warnings: string[]): void { | ||
for (const warning of warnings) { | ||
if (!WarningsCache.cache.includes(warning)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels more like a Set
than []
src/commands/plugins/inspect.ts
Outdated
@@ -122,23 +125,11 @@ export default class PluginsInspect extends Command { | |||
return {...plugin, deps: depsJson} as PluginWithDeps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this shows how much this constructed type is missing from Plugin. I think that's because of the spread operator dropping the non-public stuff from the Plugin class.
Maybe define PluginWithDeps using Pick<> or Omit<>.
dishonest/unreliable types are worse than no types. A lot of the as
in oclif seems to be indicators of that.
* refactor: hasPlugin type uses this.list types * refactor: omit class props, parallelization from TODO * refactor: warningCache use set * chore: dev-dep cleanup * chore(release): 3.9.4-qa.0 [skip ci] --------- Co-authored-by: svc-cli-bot <[email protected]>
QA notes: (3.9.4-qa.5 in 2.15.8 plugins ✅ help looks good sf plugins link (my local copy of plugin-user) sf plugins uninstall user uninstall/reinstall the uninstall/reinstall the sf plugins update not tested: |
BREAKING CHANGES: ESM and node 18 minimum
--silent
flag toplugins install
to silence yarn output (Fixes Silent yarn installation to suppress package dep warnings #196)<CLI>_NETWORK_TIMEOUT
to specify--network-timeout
on yarn operations (Fixes Allow specifying yarn network timeout during install #189)pluginPrefix
(Fixes 💡 Allow customization of the package name prefix for friendly names #462)yarn
binary every timeexec
is called (Fixes Subsequent github url installs fail withCannot find module ... yarn/bin/yarn.js
#684 @W-14331306@)plugins install
outputDepends on oclif/core#832