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

fix: correct declaration output dir #627

Merged
merged 11 commits into from
Jan 11, 2021

Conversation

zeroooooooo
Copy link
Contributor

@zeroooooooo zeroooooooo commented Dec 15, 2020

If declarationDir is special, the output dir will be unexpected, fixes #481, and I run typescript example will cause same error as follow
image

tsconfig.json as follow
image

I give a quick fix, rewrite dir before write file into disk, seems can fix the problem.

@guybedford
Copy link
Contributor

Just to clarify the expected behaviour here - do you need these types to be included in the output?

If so, the fix looks like the right sort of approach to me, but I would make two suggestions to get this merged:

  1. Include a path.isAbsolute check along with the .endsWith('.d.ts') check, and treat this purely as a renormalization based on the base directory, rather than an ad hoc folder split (which will fail on arbitrary subfolder structures).
  2. Move this logic into src/index.js where the finalizeHandler does asset completions.

@styfle
Copy link
Member

styfle commented Dec 15, 2020

Additionally, we'll need a test for this new behavior

@zeroooooooo
Copy link
Contributor Author

Just to clarify the expected behaviour here - do you need these types to be included in the output?

I think this may not be a common use case, I use this is because I need to compile code into multiple types of modules(es, commonjs), and they shared the tsconfig. to avoid duplicate generate declarations, it needs to compile declarations into a separate folder. so I make this changes, but it seems not necessary if use ncc alone.

If so, the fix looks like the right sort of approach to me, but I would make two suggestions to get this merged:

  1. Include a path.isAbsolute check along with the .endsWith('.d.ts') check, and treat this purely as a renormalization based on the base directory, rather than an ad hoc folder split (which will fail on arbitrary subfolder structures).
  2. Move this logic into src/index.js where the finalizeHandler does asset completions.

I move logic into src/index.js, but make some changes, you can check if it is ok.

@zeroooooooo
Copy link
Contributor Author

Additionally, we'll need a test for this new behavior

test added

@guybedford
Copy link
Contributor

Thanks for the test refinements here.

I'm still not super comfortable with assuming that the type folder and outDir mean the same thing. Ideally we should really read the tsconfig.json value out and use that as the reference point rather than requiring this separate option.

@zeroooooooo
Copy link
Contributor Author

zeroooooooo commented Dec 25, 2020

Thanks for the test refinements here.

I'm still not super comfortable with assuming that the type folder and outDir mean the same thing. Ideally we should really read the tsconfig.json value out and use that as the reference point rather than requiring this separate option.

Yes, it should read outDir from tsconfig. It adds the outDir path as a prefix to the asset in the final process logic in cli.js, I thought it would affect the result, so I used outDir directly, but find out it won't. Now I use outDir from tsconfig to handle the logic instead.

Thanks for the detailed review. You can check it again to see if the appropriate changes have been made.

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.

This looks great to me now, thanks for working on a comprehensive approach here.

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

Wrong output destination for declarations
3 participants