Skip to content
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

Merged
merged 43 commits into from
Oct 26, 2023
Merged

feat!: migrate to ESM #683

merged 43 commits into from
Oct 26, 2023

Conversation

mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented Oct 17, 2023

BREAKING CHANGES: ESM and node 18 minimum

Depends on oclif/core#832

BREAKING CHANGES: ESM and node 18 minimum
@git2gus
Copy link

git2gus bot commented Oct 17, 2023

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

Copy link
Member

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

nice integration tests

@@ -2,9 +2,12 @@ name: automerge
on:
workflow_dispatch:
schedule:
- cron: '17 2,5,8,11 * * *'
- cron: '42 2,5,8,11 * * *'
Copy link
Member

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?

Suggested change
- cron: '42 2,5,8,11 * * *'
- cron: '12 2,5,8,11 * * *'

@@ -22,12 +22,26 @@ jobs:
retry_wait_seconds: 60
command: npm install -g @salesforce/cli@$nightly --omit=dev
Copy link
Member

Choose a reason for hiding this comment

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

what does the $ do?

Copy link
Contributor Author

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???


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[])
Copy link
Member

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

Copy link
Member

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
while ((start.match(/node_modules/g) || []).length > 1) {
while ((start.match(/node_modules/g) ?? []).length > 1) {

paths.push(start)
async parsePluginName(input: string): Promise<string> {
if (input.includes('@') && input.includes('/')) {
input = input.slice(1)
Copy link
Member

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

} catch (error: any) {
this.error(error.message)
} catch (error: unknown) {
const {message} = error as {message: string}
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Comment on lines 104 to 109
// Fix windows bug with node-gyp hanging for input forever
// if (this.config.windows) {
// forked.stdin.write('\n')
// }
})
}
Copy link
Member

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')
Copy link
Member

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
Comment on lines 64 to 65
if (!match) return name
return match[1]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!match) return name
return match[1]
return match?.[1] ?? name

@mdonnalley mdonnalley requested a review from mshanemc October 19, 2023 21:38
Copy link
Member

@mshanemc mshanemc left a 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
Copy link
Member

@mshanemc mshanemc Oct 20, 2023

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)) {
Copy link
Member

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 []

@@ -122,23 +125,11 @@ export default class PluginsInspect extends Command {
return {...plugin, deps: depsJson} as PluginWithDeps
Copy link
Member

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.

@mshanemc mshanemc mentioned this pull request Oct 20, 2023
@mshanemc
Copy link
Member

QA notes: (3.9.4-qa.5 in 2.15.8

plugins
✅ shows plugins

✅ help looks good

sf plugins link (my local copy of plugin-user)
📓 this took a really long time on the "refreshing user plugins" step. Is that required for linking?
✅ worked, user command worked

sf plugins uninstall user
✅ worked

uninstall/reinstall the shane-sfdx-plugins
✅ both worked, install warnings look nice

uninstall/reinstall the shane-sfdx-plugins (but with --silent)
✅ both worked, silent is really nice

sf plugins update
✅ works, produces good warning about plugin-plugins not matching the CLI's version
💡 it would be interesting to know what happened from this (what did you update?)

not tested: inspect

@mshanemc mshanemc merged commit 66e3d4f into main Oct 26, 2023
6 checks passed
@mshanemc mshanemc deleted the mdonnalley/esm branch October 26, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment