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

Enable useTsconfigDeclarationDir when declarationDir #465

Closed
etienne-dldc opened this issue Jan 28, 2020 · 5 comments · Fixed by #468
Closed

Enable useTsconfigDeclarationDir when declarationDir #465

etienne-dldc opened this issue Jan 28, 2020 · 5 comments · Fixed by #468
Labels
kind: feature New feature or request

Comments

@etienne-dldc
Copy link
Contributor

etienne-dldc commented Jan 28, 2020

Current Behavior

The useTsconfigDeclarationDir in typescript2 rollup plugin is set to false.

Desired Behavior

When declarationDir is set in tsconfig.json set useTsconfigDeclarationDir to true.

Suggested Solution

useTsconfigDeclarationDir: tsconfigJSON && tsconfigJSON.declarationDir

Who does this impact? Who is this for?

This would fix #135 and in general improve experience in Monorepos (making "Go To Definition" work).

Describe alternatives you've considered

Using tsdx.config.js but the config is not easy to change and it's needed in every package of the monorepo.

@etienne-dldc etienne-dldc changed the title Enable useTsconfigDeclarationDir when declarationDir is set in tsconfig Enable useTsconfigDeclarationDir when declarationDir Jan 28, 2020
@agilgur5
Copy link
Collaborator

agilgur5 commented Jan 29, 2020

Not totally sure how this is related to #184, but in general, that solution makes sense to me as necessary to support declarationDir 👍 Can probably use optional chaining (?.) syntax now as well.

Want to add a PR w/ test for this?

@etienne-dldc
Copy link
Contributor Author

Whoops, wrong issue number: #135
Yeah, I can create a PR 😃

@etienne-dldc
Copy link
Contributor Author

It just appear to me that my "Suggested Solution" is stupid because rollup-plugin-typescript2 will ignore useTsconfigDeclarationDir if there are not declarationDir defined so my proposal is the same as setting this to true and there would be no way to have a declarationDir but still compile enerything in dist.

  1. This is fine, we just set useTsconfigDeclarationDir: true
  2. We expose a way to opt-in this option
    a. With an additional CLI argument --useDeclarationDir
    b. We read the typings field in package.json and set useTsconfigDeclarationDir if it point to the same folder as declarationDir we turn useTsconfigDeclarationDir on
    c. ???

Personally, I thing 2.a is the best options because it's non breaking and avoid unexpected behavior for people who might have a declarationDir by accident (copy/paste, default, other build tool...).

@agilgur5
Copy link
Collaborator

agilgur5 commented Jan 29, 2020

rollup-plugin-typescript2 will ignore useTsconfigDeclarationDir if there are not declarationDir defined so my proposal is the same as setting this to true

I'm not certain that this is true. rpts2 defaults declarationDir to cwd, but if useTsconfigDeclarationDir is set, it's undefined:
https://github.com/ezolenko/rollup-plugin-typescript2/blob/947da2523cf83c5ecd217439878bc28e2dd0c73f/src/get-options-overrides.ts#L29-L33

Is there some other piece of code you were looking at that made you think that? I still think your suggested solution is correct, whereas useTsconfigDeclarationDir: true seems like it would break some things due to the cwd -> undefined change.

Personally, I thing 2.a is the best options because it's non breaking and avoid unexpected behavior for people who might have a declarationDir by accident (copy/paste, default, other build tool...).

While thinking about breaking changes is a very good mentality to have when making changes to libraries (👍 👍 ), I don't think the accidental case is the important one. If someone has declarationDir set, but it's not being used, that's a bug (TSDX is not intentionally ignoring that config option).
Also tsdx create does not add a declarationDir and as a "zero-config" CLI, other build tools shouldn't be necessary with TSDX. That is to say, I'm fairly positive usage of declarationDir inside of TSDX projects is very low.
In any case, could always release as minor (but Jared controls releases, so I'm not really sure how it would end up)

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 9, 2020

Closed by #468

@agilgur5 agilgur5 closed this as completed Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants