Added ESM loading support - tests pass - MacOS / Linux / Windows works #144
Conversation
| return Object.values(m).find((m: any) => typeof m === 'function') as Hook<T> | ||
| } | ||
|
|
||
| for (const p of this.plugins) { |
There was a problem hiding this comment.
We don't have a @oclif/core version of the docs yet, but we will have to update the reference to hook execution order when we do. https://github.com/oclif/oclif.github.io/blob/docs/docs/hooks.md
| function extractClass(exported: any): HelpBaseDerived { | ||
| return exported && exported.default ? exported.default : exported | ||
| } | ||
|
|
||
| export function getHelpClass(config: IConfig): HelpBaseDerived { | ||
| export async function getHelpClass(config: IConfig): Promise<HelpBaseDerived> { |
There was a problem hiding this comment.
Can we change this to fetchHelpClass now that it is async?
| @@ -11,6 +11,7 @@ | |||
| "cli-ux": "^5.1.0", | |||
| "debug": "^4.1.1", | |||
| "fs-extra": "^9.0.1", | |||
| "get-package-type": "^0.1.0", | |||
There was a problem hiding this comment.
I don't love shipping with 0.x releases. I took a look in the repo and I can't tell if there are plans to have a 1.x release. They might be waiting for a node equivalent?
| * @returns {Promise<{isESM: boolean, module: *, filePath: string}>} An object with the loaded module & data including | ||
| * file path and whether the module is ESM. | ||
| */ | ||
| static async loadGetData(config: IConfig|IPlugin, modulePath: string) : Promise<{isESM: boolean, module: any, filePath: string}> { |
There was a problem hiding this comment.
I think this name is a little weird, maybe loadWithData or loadWithInfo?
| @@ -73,6 +74,8 @@ export class Plugin implements IPlugin { | |||
|
|
|||
| pjson!: PJSON.Plugin | |||
|
|
|||
| isESM = false | |||
There was a problem hiding this comment.
Can't we completely hide this in ModuleLoader and just call ModuleLoader.load() here instead? The getPackageType call in the ModuleLoader code should resolve the correct pjson.type. To potentially reduce file io, you could also pass in pjson.type to the ModuleLoader.load method maybe?
|
@typhonrt I did an initial review. Let me know your thoughts. |
|
Apologies for the delay. I was deep in refactoring several core modules for ESM / ES2020 / 21 support. I'm readily available going forward through the Oclif v2 launch. I'll answer all queries from above inline:
The hook execution order is the same with the changes (everything happens in sequential order). Documentation additions should cover that you can resolve an actual package name as a hook or from Node 12.16+ which supports the exports property in
I'm definitely open to all changes regarding name, structure, and formatting; glad to get this code into Oclif v2!
Re: get-package-type, indeed is a bit of stopgap module examining the source though it is being used by I didn't want to be so brazen as to try and include my own module that handles the same task. I have a significantly more robust module that is built for easier ESM usage, IE supports file URLs / Even if a Node API equivalent became available this wouldn't be available Node 12+, so some solution like
I definitely like the
I do agree that this can be moved to ModuleLoader. In this case it might be best to check the Now I think the best way to proceed is to actually make a new PR. I just want to be absolutely sure there is a clean merge process since there are some conflicting files now with the main branch. I do not see any particular overlapping conflict. I'll cleanup all of the suggested changes above in addition to adding the quality of life changes I mentioned in #130. The one area I would like to get your input is in the help subsystem going fully asynchronous. This change is not in the initial PR as also mentioned in #130. While I rewrote ./src/help/index.ts in ESM I have fully prototyped these changes in my middleware as you can see here. The changes are for I'll get started on the new feature branch and test things tomorrow. Do let me know if this sounds like a good way to proceed. Regards... |
Sounds good. I'll close this PR. If I remember correctly, I agree with the quality of life changes but it might be better to do separate PRs: one for the ESM then one for quality of life. It will make the PRs easier to digest and focused if they don't depend on each other.
I don't see a problem with this. Let's definitely keep this in a separate PR. We will also be adding / changing several help class features in the next month, so I can also make these changes if you want. |
|
many thanks for this enhancement! |
See #130 for more information. Creating the PR for comments, testing, and further discussion.