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

Change source code to TypeScript #133

Merged
merged 20 commits into from
Jul 13, 2020
Merged

Change source code to TypeScript #133

merged 20 commits into from
Jul 13, 2020

Conversation

styfle
Copy link
Member

@styfle styfle commented Jul 8, 2020

I found a few bugs when changing the source code to TypeScript.

I'll need some feedback from @guybedford to decide how to proceed in some cases, see comments below.

@styfle styfle requested a review from guybedford July 8, 2020 04:48
@styfle styfle requested review from lucleray and Timer as code owners July 8, 2020 04:48
src/analyze.ts Outdated Show resolved Hide resolved
src/resolve-dependency.ts Outdated Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/analyze.ts Outdated Show resolved Hide resolved
src/analyze.ts Show resolved Hide resolved
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Great work on this! Are the AST Node types coming through at all?

src/analyze.ts Outdated Show resolved Hide resolved
src/analyze.ts Outdated Show resolved Hide resolved
src/analyze.ts Show resolved Hide resolved
src/resolve-dependency.ts Outdated Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/utils/get-package-base.ts Show resolved Hide resolved
src/utils/get-package-base.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/analyze.ts Show resolved Hide resolved
src/resolve-dependency.ts Outdated Show resolved Hide resolved
if (typeof exports === 'string' ||
typeof exports === 'object' && !Array.isArray(exports) && Object.keys(exports).length && Object.keys(exports)[0][0] !== '.')
exports = { '.' : exports };
function resolveExportsTarget (pkgPath: string, exp: string | { [key: string]: string }, subpath: string, job: Job, cjsResolve: boolean): string | undefined {
Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that getExportsTarget() handles the exports: null case however this resolveExportsTarget() function does not. Should we also handle exports: null here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes on 117 we should probably have a null check too for consistency.

Copy link
Member Author

@styfle styfle Jul 12, 2020

Choose a reason for hiding this comment

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

What should happen for null on line 117?

  • return null?
  • assign exports = { '.' : null }?
  • assign exports = null?
  • something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually exports can never be null or undefined here as it is guarded in all calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I added a PkgCfg and Exports type so we can confirm all cases are covered, thanks!

src/analyze.ts Show resolved Hide resolved
@@ -183,5 +200,5 @@ function resolvePackage (name, parent, job, cjsResolve) {
return resolveFile(pathTarget, parent, job) || resolveDir(pathTarget, parent, job);
Copy link
Contributor

Choose a reason for hiding this comment

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

This path should have || throwNotFound on the fallback.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ha, this is the missing piece! I pushed a change which makes sure resolveDependency(): string | string[] so we never return undefined anymore ☺️

@styfle styfle requested a review from guybedford July 13, 2020 14:54
@styfle styfle added the automerge Automatically merge PR once checks pass label Jul 13, 2020
@kodiakhq kodiakhq bot merged commit 3c179ef into master Jul 13, 2020
@kodiakhq kodiakhq bot deleted the use-typescript branch July 13, 2020 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PR once checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants