Enable useTsconfigDeclarationDir if declarationDir by etienne-dldc · Pull Request #468 · jaredpalmer/tsdx

etienne-dldc · 2020-01-29T22:40:34Z

Fixes #465

This enable thes useTsconfigDeclarationDir option of rollup-plugin-typescript2 when declarationDir is set in tsconfig.

fix (probably) #135 too

etienne-dldc · 2020-01-29T22:55:20Z

CI fails because "Your lockfile needs to be updated, but yarn was run with --frozen-lockfile." should I run yarn and update the PR ?

agilgur5 · 2020-01-29T23:23:46Z

CI fails because "Your lockfile needs to be updated, but yarn was run with --frozen-lockfile." should I run yarn and update the PR ?

If you rebase with master and force-push, it should be fixed. It was just fixed in #470

agilgur5

Awesome you made the test folder too! Was going to do that myself 😄

Looks good, but a few changes:

Can you rename it to build-withTsconfig (directory, package.json, unit test)? We can re-use the fixture for testing other custom tsconfig properties (e.g. #469 ) similarly to how build-default is used for a few different tests.
Can you add this directory to the list in test/fixtures/README.md?
syntax/ folder and foo.ts are unnecessary/redundant for this case. just have the small index.ts is plenty (also only its declaration file is checked for in the test anyway)

agilgur5 · 2020-01-29T23:38:13Z

@@ -0,0 +1,28 @@
+{
+  "compilerOptions": {
+    "target": "es5",


target was just removed from all test fixtures in #466

agilgur5

Thanks for the PR and quick iteration!

justingrant · 2020-02-02T21:54:20Z

@etienne-dldc @agilgur5 - if declarationDir is omitted, then useTsconfigDeclarationDir will be false, which in turn will trigger #479. This is different from tsc's behavior which is that outDir will be used to emit declaration files. I think it's probably a bad idea for default tsdx behavior to be different from default tsc behavior for the same config.

What do you think about making useTsconfigDeclarationDir true by default in rollup config, even if there's no declarationDir? If yes, then this tsdx config setting could be an opt-out to handle cases where that default is not desired.

I read @agilgur5's comment in #465, but I just ran rollup with no declarationDir and also with useTsconfigDeclarationDir: true. Rolllup emitted all files in the right place and emitted correct sourcemap and declaration map files. So I'm not sure what bad outcome would happen if useTsconfigDeclarationDir: true was default.

agilgur5 · 2020-02-02T22:04:58Z

This is different from tsc's behavior which is that outDir will be used to emit declaration files.

@justingrant I don't think this specific PR is the right place to discuss outDir because TSDX doesn't really support outDir as I mentioned in #479 (comment).
We can add support for that separately. This PR is specific to declarationDir support and would prefer not to side-track it (especially as outDir is a MUCH larger issue in TSDX)

I read @agilgur5's comment in #465, but I just ran rollup with no declarationDir and also with useTsconfigDeclarationDir: true. Rolllup emitted all files in the right place and emitted correct sourcemap and declaration map files. So I'm not sure what bad outcome would happen if useTsconfigDeclarationDir: true was default

I do think we'd want testing around this because rpts2 does behave differently when it is set vs. unset, as shown by the rpts2 code I pointed to in my comment.
Did that work with outDir set? Or with no outDir and no declarationDir?? You changed your repro and didn't provide a specific branch here, so the specifics of when it worked is opaque and confusing me, sorry

justingrant · 2020-02-03T06:11:38Z

@agilgur5 - oops, I didn't notice that tsdx doesn't support outDir. Probably because my outDir was just ./dist which I assume is the default. I agree with you that outDir-related questions are out of scope for this PR. Good point.

That said, I still think my question is relevant: would it be good for useTsconfigDeclarationDir to be true by default, regardless of whether declarationDir is present in tsconfig? What problems can happen if useTsconfigDeclarationDir is true but declarationDir is missing?

Did that work with outDir set? Or with no outDir and no declarationDir?? You changed your repro and didn't provide a specific branch here, so the specifics of when it worked is opaque and confusing me

Sorry for confusion. I observed that if useTsconfigDeclarationDir is true (I ensured this by hacking tsdx's createRollupConfig.js) then it didn't matter whether declarationDir was ./dist, dist, dist/, or was not present. In all those cases, tsdx correctly emitted index.d.ts into dist and index.d.ts.map was emitted with correct sources paths. While running these tests, outDir was set to ./dist, so I'm not sure about behavior if there's no outDir. You are right that tests are needed to cover all the cases. Let me know if there's more clarification I could provide here about the behavior I observed.

agilgur5 · 2020-02-03T14:20:29Z

@agilgur5 - oops, I didn't notice that tsdx doesn't support outDir. Probably because my outDir was just ./dist which I assume is the default. I agree with you that outDir-related questions are out of scope for this PR. Good point.

Yea outDir set to anything else won't actually make it output to a different directory and also multiple files will not be output correctly (iirc they'll overwrite each other since there's some name hard-coding). It is still passed to rpts2 which passes it to TS Language Service, so it does have some effect as you've seen.
(There's #321 specifically for outDir and then like 10 issues related to outDir, preserveModules, and multi-entry; pretty sure these are the most popular issues by a wide margin right now. My #367 goes a good amount of the way toward solving some, but not all of these problems. It's quite non-trivial as well, so digging into those weeds is a lot for this comparatively extremely small PR 😅 )

That said, I still think my question is relevant: would it be good for useTsconfigDeclarationDir to be true by default, regardless of whether declarationDir is present in tsconfig? What problems can happen if useTsconfigDeclarationDir is true but declarationDir is missing?

I'll refer to my proposed fix in #479 (comment) for any other readers.
Idk what can happen, hence the anxiety around it 😅 The code paths are different, so don't want to change that and potentially break people's builds without a fix or ample testing. The proposed fix I reference above would add a default declarationDir, so that would ensure the same code path regardless. No changes required here in this PR.

I observed that if useTsconfigDeclarationDir is true (I ensured this by hacking tsdx's createRollupConfig.js) then it didn't matter whether declarationDir was ./dist, dist, dist/, or was not present. In all those cases, tsdx correctly emitted index.d.ts into dist and index.d.ts.map was emitted with correct sources paths. While running these tests, outDir was set to ./dist, so I'm not sure about behavior if there's no outDir. You are right that tests are needed to cover all the cases.

Ok, so they were both only ever set to dist/ and outDir was always set. Thanks for the summary. That covers no declarationDir and yes outDir case. When neither is set but useTsconfigDeclarationDir is set is another that is potentially problematic. In any case, my proposed fix should workaround any potential problems and can handle outDir at a later date.

No changes required here in this PR.

agilgur5

So when working on #488 , I noticed some confusing changes here.

diff test/fixtures/build-default/tsconfig.json test/fixtures/build-withTsconfig/tsconfig.json should have only shown the necessary changes here, that declarationDir and declarationMap are added, but some others popped up that confused me for a bit while debugging.

Not totally sure why these configs were changed as they don't seem to have any impact on the output, but maybe I'm missing something as it's my first time actually playing with these options.

Also, #488 is a decent amount more complex (including a breaking change) than just setting useTsconfigDeclarationDir: true at all times, so good thing it was separated out 😅

agilgur5 · 2020-02-12T04:25:44Z

Welp this still hasn't been merged by Jared and #504 got merged in first (that one is high prio to be fair), so now rootDir needs to be set back to './src'

Annd there's a merge conflict in here from #476 which also got merged first before this 😭

SORRY for the churn, I'd have merged this in a while ago if I could 😐

Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>

etienne-dldc · 2020-02-18T23:06:05Z

I've messed up a little bit with git (sorry 😞 ) but should be good now...

Ready to merge again 😉

etienne-dldc · 2020-02-25T08:29:54Z

👋 @jaredpalmer any update on this ?

jaredpalmer · 2020-02-26T14:10:30Z

Sorry for churn! Thank you for the effort! Makes a lot of sense. Would you be able to summarize this in one shirt comment for the release notes for me?

etienne-dldc · 2020-02-26T15:59:19Z

Sure 😃,

Tsdx now uses declarationDir when found in tsconfig.json. This let you change where types declarations (and associated .d.ts.map) are emitted. It also allow you to get correct .d.ts.map paths by setting declarationDir to dist.
This PR fix #135 & #465

If this is too long you can keep just the first two sentences or even just the first one.

agilgur5 · 2020-02-27T01:29:59Z

Just one thing -- this doesn't completely fix #135 / #479. As I wrote in #479 and a bit here, this creates a workaround for correct declaration map sources, but doesn't resolve the default case. That is resolved by #488 (and #504 which was split off it); though it's kind of an upstream problem more broadly: ezolenko/rollup-plugin-typescript2#204

agilgur5 · 2020-03-09T18:14:43Z

I emailed Jared and have now been added as a collaborator so I've merged this myself now. I don't yet have permissions on NPM to publish though.

Thanks again for this change @etienne-dldc and sticking through with the delays and conflicts 🙏 💯 😄

@all-contributors please add @etienne-dldc for bugs, code, tests

allcontributors · 2020-03-09T18:14:54Z

@agilgur5

I've put up a pull request to add @etienne-dldc! 🎉

TSDX now uses `declarationDir` when found in `tsconfig.json`. This let you change where types declarations (and associated declaration maps) are emitted. It also allow you to get correct declaration maps (`.d.ts.map`) paths by setting `declarationDir` to the default `dist` output directory * Enable useTsconfigDeclarationDir if declarationDir * Rename fixture folder to be reused * Update test/fixtures/build-withTsconfig/tsconfig.json Co-Authored-By: Anton Gilgur <agilgur5@gmail.com> * Update test/fixtures/build-withTsconfig/tsconfig.json Co-Authored-By: Anton Gilgur <agilgur5@gmail.com> * Fix tests * Enable useTsconfigDeclarationDir if declarationDir * Rename fixture folder to be reused * Set rootDir to src and fix test Co-authored-by: Anton Gilgur <agilgur5@gmail.com>

etienne-dldc mentioned this pull request Jan 29, 2020

Esmodule options in rollup output is always undefined #469

Closed

etienne-dldc force-pushed the master branch from d4a884c to b227400 Compare January 29, 2020 23:32

agilgur5 requested changes Jan 29, 2020

View reviewed changes

agilgur5 approved these changes Jan 29, 2020

View reviewed changes

swyxio requested a review from jaredpalmer January 30, 2020 01:32

This was referenced Jan 31, 2020

docs: add agilgur5 as a contributor #446

Merged

sources paths are incorrect in declarationMap files (*.d.ts.map) emitted by tsdx #479

Closed

justingrant mentioned this pull request Feb 2, 2020

tsdx silently fails parsing of tsconfig files containing comments, trailing commas, etc. that TSC accepts #483

Closed

justingrant mentioned this pull request Feb 4, 2020

tsdx ignores tsconfig.json "extends" keyword #484

Closed

agilgur5 mentioned this pull request Feb 4, 2020

(fix): declarationMaps (*.d.ts.map) should have correct sources #488

Open

agilgur5 requested changes Feb 4, 2020

View reviewed changes

Comment thread test/fixtures/build-withTsconfig/tsconfig.json

Comment thread test/fixtures/build-withTsconfig/tsconfig.json Outdated

agilgur5 approved these changes Feb 5, 2020

View reviewed changes

This was referenced Feb 7, 2020

(fix/refactor): rewrite some overbroad try/catches #500

Closed

(fix): set rootDir to './src', not './'. deprecate moveTypes #504

Merged

etienne-dldc and others added 7 commits February 18, 2020 23:41

Enable useTsconfigDeclarationDir if declarationDir

34f63a9

Rename fixture folder to be reused

a9f0825

Update test/fixtures/build-withTsconfig/tsconfig.json

7600884

Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>

Update test/fixtures/build-withTsconfig/tsconfig.json

f79a2db

Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>

Fix tests

c40de21

Enable useTsconfigDeclarationDir if declarationDir

7711743

Rename fixture folder to be reused

dc68d10

etienne-dldc force-pushed the master branch from 7e634a8 to dc68d10 Compare February 18, 2020 22:43

Set rootDir to src and fix test

7b7ff29

agilgur5 merged commit b23f158 into jaredpalmer:master Mar 9, 2020

allcontributors bot mentioned this pull request Mar 9, 2020

docs: add etienne-dldc as a contributor #548

Merged

This was referenced Mar 9, 2020

Enable useTsconfigDeclarationDir when declarationDir #465

Closed

(fix): parse tsconfig extends, trailing commas, and comments #489

Merged

agilgur5 mentioned this pull request Mar 17, 2020

(refactor): split build tests into separate files per fixture #621

Merged

Conversation

etienne-dldc commented Jan 29, 2020 • edited by agilgur5 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etienne-dldc commented Jan 29, 2020

Uh oh!

agilgur5 commented Jan 29, 2020

Uh oh!

agilgur5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agilgur5 Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

agilgur5 left a comment

Choose a reason for hiding this comment

Uh oh!

justingrant commented Feb 2, 2020

Uh oh!

agilgur5 commented Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justingrant commented Feb 3, 2020

Uh oh!

agilgur5 commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agilgur5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

agilgur5 commented Feb 12, 2020

Uh oh!

etienne-dldc commented Feb 18, 2020

Uh oh!

etienne-dldc commented Feb 25, 2020

Uh oh!

jaredpalmer commented Feb 26, 2020

Uh oh!

etienne-dldc commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agilgur5 commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agilgur5 commented Mar 9, 2020

Uh oh!

allcontributors bot commented Mar 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

etienne-dldc commented Jan 29, 2020 •

edited by agilgur5

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 commented Feb 2, 2020 •

edited

Loading

agilgur5 commented Feb 3, 2020 •

edited

Loading

agilgur5 left a comment •

edited

Loading

etienne-dldc commented Feb 26, 2020 •

edited

Loading

agilgur5 commented Feb 27, 2020 •

edited

Loading