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: read extended tsconfigs #845

Merged
merged 18 commits into from
Nov 2, 2023
Merged

feat: read extended tsconfigs #845

merged 18 commits into from
Nov 2, 2023

Conversation

mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented Oct 27, 2023

  • Pass entirety of tsconfig to tsNode.register
  • Use tsconfck instead of typescript for reading/merging extended tsconfigs

@mdonnalley mdonnalley changed the title feat: add swc option if possible feat: read extended tsconfigs Oct 30, 2023
@mdonnalley mdonnalley added the enhancement New feature or request label Oct 30, 2023
@git2gus
Copy link

git2gus bot commented Oct 30, 2023

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

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.

requested changes are the public api questions and interface stuff

return ids
}

public async getCommandsDir(): Promise<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.

do consumers of Plugin need this method publicly exposed?

@@ -162,8 +150,25 @@ export class Plugin implements IPlugin {
return cmd
}

public async getCommandIDs(): Promise<string[]> {
Copy link
Member

Choose a reason for hiding this comment

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

do consumers of Plugin need this method publicly exposed? Or is the commandIDs property enough?

Copy link
Member

Choose a reason for hiding this comment

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

they're not in the interface so maybe private or #getCommandIDs is appropriate?

or a function that takes the name and commandsDir ?

Comment on lines 38 to 40
const final = {...result.tsconfig, 'ts-node': tsNodeOpts}

TS_CONFIGS[root] = final
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
const final = {...result.tsconfig, 'ts-node': tsNodeOpts}
TS_CONFIGS[root] = final
TS_CONFIGS[root] = {...result.tsconfig, 'ts-node': tsNodeOpts}

@@ -38,7 +38,7 @@ export async function load<T = any>(config: IConfig | IPlugin, modulePath: strin
let filePath: string | undefined
let isESM: boolean | undefined
try {
;({filePath, isESM} = resolvePath(config, modulePath))
;({filePath, isESM} = await resolvePath(config, modulePath))
Copy link
Member

Choose a reason for hiding this comment

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

what's the semicolon for?

Copy link
Contributor Author

@mdonnalley mdonnalley Oct 31, 2023

Choose a reason for hiding this comment

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

Not sure to be honest - it's a default prettier thing

await test('Install CJS plugin to CJS root plugin', async () => {
await installTest(PLUGINS.cjs2, cjsExecutor)
})
// await test('Install CJS plugin to CJS root plugin', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

why are these commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commented out for iterating on tests - thanks for catching!

export class Plugin implements IPlugin {
alias!: string

alreadyLoaded = false

children: Plugin[] = []

commandIDs!: string[]
Copy link
Member

Choose a reason for hiding this comment

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

doesn't the code below break the interface? When it was a getter, it would always return something. The ! means that it's not possible to have a Plugin (class) without commandIDs, but without calling load() you would.

Maybe init them as empty arrays and then load can fix them if it runs?

Comment on lines 31 to 36
let tsNodeOpts = {}
for (const extended of (result.extended ?? []).reverse()) {
if (extended.tsconfig['ts-node']) {
tsNodeOpts = {...tsNodeOpts, ...extended.tsconfig['ts-node']}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

not 100% sure this works, but fromEntries() is LWW so you don't have to reverse the array to get the last ones in first for Object.assign.

const tsNodeOpts = Object.fromEntries(
        (result.extended ?? [])
          .map((e) => Object.entries(e.tsconfig['ts-node'] ?? {}))
      )

commands!: Command.Loadable[]

commandsDir!: 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.

I don't understand what ! means if there's also string | undefined. Seems like an contradiction?

this.hooks = {}
for (const [k, v] of Object.entries(this.pjson.oclif.hooks ?? {})) {
// eslint-disable-next-line no-await-in-loop
this.hooks[k] = await Promise.all(castArray(v).map(async (i) => tsPath(this.root, i, this)))
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 have a good sense for how long tsPath takes to run, or how many hooks there could be per plugin, but what's the reason for not parallelizing these?

Copy link
Member

Choose a reason for hiding this comment

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

this.hooks = Object.fromEntries(await Promise.all(
      Object.entries(this.pjson.oclif.hooks ?? {})
        .map(async ([k, v]) => [k, await Promise.all(
          castArray(v).map(async (i) => tsPath(this.root, i, this))
        )])
    ))

mshanemc
mshanemc previously approved these changes Nov 1, 2023
@mshanemc
Copy link
Member

mshanemc commented Nov 1, 2023

QA notes:

linked this PR into plugin-user to use bin/dev (cjs),

./bin/dev org:list:users -o hub
✅ works

set ./tsconfig.json to have

{ 
  "ts-node": {
    "transpileOnly": true
  }
}

DEBUG=* ./bin/dev org:list:users -o hub to check out the debug logs
✅ I see config:ts-node transpileOnly: true,


in main/top-level tsconfig, showConfig:true. In what it extends (dev-config/tsconfig-strict.json) it's false

DEBUG=* ./bin/dev org:list:users -o hub to check out the debug logs
❌ I see config:ts-node showConfig: false,

I think I was expecting true...like the top-level one extends other ones, and should override them. So maybe that one needs to get added to the result.extended OR merged in at the end/

@mshanemc
Copy link
Member

mshanemc commented Nov 2, 2023

QA round 2:

linked into plugin-user.

All permutations of top-level tsconfig, plus local modifications to dev-config/tsconfig-strict which extends dev-config/tsconfig.

✅ all working with proper results when toggling a single ts-node prop (showConfig)
✅ can add other props in "lower" levels and have them preserved when higher levels don't include that one.

rm -rf lib
✅ is running from ts

linked plugin into sf
✅ works

@mshanemc mshanemc merged commit 59145ee into main Nov 2, 2023
34 checks passed
@mshanemc mshanemc deleted the mdonnalley/swc branch November 2, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants