include in tsconfigDefaults is overridden by index from original tsconfig.json

[tarsisexistence]

What happens and why it is wrong

That bug is really weird.
At some point, I started getting a weird error caused by include option of typescript2 config and project tsconfig.json.
I even stopped understanding how it works, does it override or concatenate, or what?
then, I decided that I need to debug this array to check what finally gets there

My rollup-plugin-typescript2 config:

  typescript2({
      clean: true,
      tsconfigDefaults: {
        ....
        include: [path.resolve(__dirname, 'styles.d.ts')],
      }
    }),

My tsconfig.json config:

   ...
  "include": ["src", "declaration.d.ts"]
}

The result is

As I said before, I started debugging this it gives some kind of awkward result every time.
In fact, it does not concatenate these properties, but replaces the values in the index cell, which simply takes by surprise and worsens developer experience.

What I mean:
it takes 0 the index of the array of my tsconfig and puts it instead of the value under the 0 index of typescript2 config.

Example 1:

typescript2: ['a']
tsconfig: ['b', 'c']
result: ['b', 'c']

Example 2:

typescript2: ['a', 'd']
tsconfig: ['b', 'c']
result: ['b', 'c']

Example 3:

typescript2: ['a', 'd', 'e']
tsconfig: ['b', 'c']
result: ['b', 'c', 'e']

It is completely annoying

Environment

I'm not sure this is relevant, but
Node: 13.13.0
OS: macOS Catalina 10.15.4

Versions

• typescript: 3.8.3
• rollup: 2.8.1
• rollup-plugin-typescript2: 0.27.0

[tarsisexistence]

this makes using include impossible since you never know how long the array will be in the tsconfig.json

[tarsisexistence]

Interesting thing,
if I movetypescript2 config include option from tsconfigDefaults to tsconfigOverride
it will override original tsconfig.json include option by index.

that is, it is exactly the opposite
but it’s also very bad

[tarsisexistence]

and one more thing
after some additional investigations
I realized that the same behavior is relevant for other array properties as well

[agilgur5]

So I've hit this issue before in jaredpalmer/tsdx#666 (comment), and per that issue, that's just how _.merge's deep merge works. It's a deep merge so it merges the arrays and the index is the only identifier of the array. It may be unexpected but it is a deep merge, that's how deep merges work.

In fact, it does not concatenate these properties, but replaces the values in the index cell,

• Instead doing an append/concat wouldn't be merging at all, and would give no way to overwrite with tsconfig at all. That seems like the wrong behavior to me.
• Instead doing a shallow merge would just overwrite what's in defaults, which I'm not sure is expected behavior either, but it is similar to how tsconfig works with regard to exclude: if you add an exclude, it overwrites the defaults.

I could write up a PR to do a shallow merge on just include and exclude, but that would be pretty breaking of a change; I have no idea how that might impact all downstream users. Idk if TSDX users expect a shallow or deep merge, but it would break their usage if expecting deep.

if I movetypescript2 config include option from tsconfigDefaults to tsconfigOverride
it will override original tsconfig.json include option by index.

that is, it is exactly the opposite

Yes, that is how tsconfigOverride is supposed to work, it is an override.

I realized that the same behavior is relevant for other array properties as well

Yep, that is how _.merge does a deep merge, so anything in tsconfig that is an array has the same change.

[tarsisexistence]

@agilgur5 thanks for response!

1. From my point of view, this behavior makes configuration totally useless. We cannot guarantee the reliability of the config if we are tied to a position in the array.

2. Indeed, it'd be a breaking change. But if our direction is to achieve the best DX, then we should make this thing clear and useful.

I have a trick in my pocket to handle this thing, but it looks noisy from the clean code perspective - just read tsconfig, get include property and concatenate this with my array. It looks like it should be easier.

My suggestion:

1. for default - collect the default property and concatenate with the original tsconfig; remove duplicates.
2. for override - apply only override option

[agilgur5]

Defaults do not normally concatenate when overridden, they get overridden. And as I mentioned above, concatenation makes it impossible to override the default with a tsconfig. That sounds like wrong behavior to me in multiple ways, so I strongly discourage that.

I think all of the options are confusing, I'm not sure I agree that any of these are the "best DX". A shallow merge is the closest thing to how tsconfig works with its default exclude, but that does not come without trade-offs.
In general, breaking changes need to have a lot of thought put into them, which is why I brought that up -- the downstream effects are non-trivial; TSDX would also have to release a breaking change to update to such a change.

[tarsisexistence]

@agilgur5 fine, then I can’t use either default or override at all.
Cause? There is no guarantee that no value will be written on that index.
Plus, you can get around all via undefined as a value of the array, and adding your needed value further. And it turns out that there is a gap in this system? Just great.

The case:

1. I want to add my additional d.ts typings inside "include" config property.

2. The user puts his src dir inside "include" with another project specific d.ts typings outside src.

3. ['config.d.ts`]
   2.['src', 'some.d.ts', 'another.d.ts']

4. Result: ['src', 'some.d.ts', 'another.d.ts']

As you can see, there is no way to configure this properly. I should read tsconfig.json, get its "include" property and merge this into ['src', 'some.d.ts', 'another.d.ts', 'config.d.ts'].
And I think this is a complete overhead.

Meanwhile, I figured out what the problem is. Others may not figure it out and spend their precious time to figure out what doesn’t work for them.
And after your first message, I have already seen at least 2 related issues. And it will go on and on.

I see it so that if we leave everything as it is, then I can’t use it, it’s a black box for me because I can't guarantee anything.
There is no room for personal preferences "like" / "dislike".
This can either be used or not. And it's can't.

I have no problem gathering the opinions of others who are involved in this.
And one more thing, I have no problems to stay with the current solution, I have the hack described above, which I am ready to stick with. But I don’t understand what the problem is of making this solution transparent, without being tied to the fact that it will entail breaking changes.

[agilgur5]

I mean, you're giving your own preference here while I've given multiple different options already. I've already said twice why that preference, concatenation, is fundamentally broken, similarly unintuitive, and not at all "transparent". To repeat, concatenation isn't just a breaking change, it literally makes overriding things impossible. I'll repeat again, impossible.
It being impossible to override with concatenation is not a "preference", that is a fact.

Again, a "default" that can't be overridden isn't a default, that's a requirement. Changing a "default" to a requirement doesn't make sense and is wrong behavior.

You can override the deep merge right now, you cannot override a concatenation. Making an existing use-case impossible doesn't make sense.

I've also already given an alternative option twice already that isn't fundamentally broken, which is a shallow merge. I'll repeat again, you can use a shallow merge to solve this too.
Whether that's better is debatable. There are trade-offs to everything.

I have contributed here to fix bugs before (I know it's _.merge because I've read the codebase) and a library I maintain makes up a significant portion of usage of this library (~10%), I'm not just making talking points here...

Not to mention, this can be resolved without breaking changes -- you can add a new option for tsconfigConcat or tsconfigDefaultShallow etc.

To say that there is only one option and it must be breaking and must make existing use-case impossible is simply not true.

[ezolenko]

I think we'll have to break things no matter what we do here. To make it behave according to the spirit of documentation and property names it should do basically this:

• tsconfigDefaults - this is the object everything below will be deeply merged into
• tsconfig - this is the object that we get from typescript, after its own imports and everything. All values here replace values in tsconfigDefaults. All arrays here are concatenated with arrays in tsconfigDefaults.
• tsconfigOverride - this is the nuclear option. Objects are still merged deeply, but arrays here replace arrays wholesale. So if you provide an empty include array here, final tsconfig will have no includes at all.

Will that cover all the cases we want to support (after users make appropriate changes), or am I missing something?

[tarsisexistence]

@ezolenko sounds amazing and makes a lot of sense to use

[agilgur5]

@ezolenko I'm not sure why you have to break things? I've given options that are non-breaking already.

I'm not sure how what you've listed reflects "the spirit of the documentation and property names" because the docs say:

The object passed as tsconfigDefaults will be merged with loaded tsconfig.json. Final config passed to typescript will be the result of values in tsconfigDefaults replaced by values in loaded tsconfig.json
[...]
This is a deep merge (objects are merged, arrays are concatenated, primitives are replaced, etc), increase verbosity to 3 and look for parsed tsconfig if you get something unexpected.

It says "merged", "replaced", and "deep merge", all of which mean replace and override the default. There is a single reference to "concatenated" that is different from the rest, but a deep merge does not in fact concatenate. 5 references say one thing and a 6th is incorrect. To me, that sounds like the 6th reference that is incorrect should be corrected, not the 5 correct ones changed to the incorrect 6th.

lodash is the de facto standard and its definition of merge is not to concatenate. The definition of defaults is that they are overridden when you set the same property value, not concatenated:

_.defaults({ a: ['a'] }, { a: ['b', 'c'] });
// => {a: ['a']}
_.defaultsDeep({ a: ['a'] }, { a: ['b', 'c'] });
// => {a: ['a', 'c']}

If you'd like to add a concatenation option, I think that should be a separate configuration, because it does not match 5+ references in the docs either and because that is simply not what defaults means, neither in definition nor in usage in any library.

It also breaks the symmetry with tsconfigOverride as well as its' own docs:

• tsconfigOverride: {}
  See tsconfigDefaults.

I expect defaults to be overridden, that is the definition, making them concatenate instead is incredibly unintuitive. A deep merge may be unintuitive for arrays, but the docs plainly say it is a deep merge and it was quick for me to understand that was the issue because indexes were relevant. I also did not report a bug because that is what the docs say and even link to deep merge -- that's intended behavior, not a bug.

As I've said, shallow merging the arrays may be more intuitive, as that is how tsconfig itself actually works. As I've linked above, tsconfig itself does not concatenate when you add an exclude, it overrides the existing exclude.

So concatenate doesn't match 5+ references in the docs already, the definition of merge, the definition of defaults, nor tsconfig's own behavior. And it breaks symmetry and docs of other options. I'm not sure how that is intuitive.

And as I've said multiple times, making it a very unintuitive concatenation instead of overriding makes certain uses cases impossible. Here is TSDX's usage:

https://github.com/jaredpalmer/tsdx/blob/17ffcd215f78a4e9d6936644cbeab332f6439088/src/createRollupConfig.ts#L149-L178

The intent is that if you add your own exclude, it overrides the defaults we've set. The defaults are only applied if you haven't set the property name, just like every other property name (again, that is the definition of defaults and how defaults work in all libraries).

If you were to change that to concatenate, then the user now has no way of overriding our default exclude, it is impossible for them to override the default. We ship tsconfig's default exclude as well, so this also makes it impossible to override tsconfig's default, even though tsconfig itself lets you do that.

Not sure why I'm talking like a broken record here repeating existing definitions and existing usage in the whole ecosystem.

[tarsisexistence]

@agilgur5

And as I've said multiple times, making it a very unintuitive concatenation instead of overriding
makes certain uses cases impossible. Here is TSDX's usage:
https://github.com/jaredpalmer/tsdx/blob/17ffcd215f78a4e9d6936644cbeab332f6439088/src/createRollupConfig.ts#L149-L178

This configuration can be disrupted easily, and I don’t understand why this is not obvious to you. I don't want my library to be fragile, and that's why I created this issue.

the funny thing is that @ezolenko and I have already proposed a solution that will help protect, including TSDX, but you still resist.

The defaults are only applied if you haven't set the property name

I do agree with default behavior. It makes sense to apply config default only when tsconfig does not have corresponding properties, but this gives rise to many other, more complex problems, the solution of which will overcomplicate API.

impossible for them to override the default

The question is, why should the user have access to what the author of the library forbade?

We must have leverage tsconfig configuration that cannot be overwritten, but at the same time, we need to give the opportunity to expand our presets. And this is the main point.

The solution lies only in the design of the new API.

[agilgur5]

This configuration can be disrupted easily, and I don’t understand why this is not obvious to you. I don't want my library to be fragile, and that's why I created this issue.

You're taking words out of my mouth that I didn't remotely say. I did not say it can't and nor did I say it wasn't fragile. I just said that your solution doesn't match any existing definition nor existing usage in the whole ecosystem of defaults nor the docs themselves. Breaking the entire definition of default is clearly wrong behavior.
As I've said several times, my suggestion was to shallow merge arrays instead, i.e. tsconfig overrides tsconfigDefaults as it does for every non-array property and as tsconfig itself works.
As I've said several times, that can be done with a breaking change or a non-breaking change.

Please don't take words out of my mouth. I've repeated myself numerous times, please read the actual things I wrote instead of making things up.

The question is, why should the user have access to what the author of the library forbade?

tsconfig didn't forbid that, TSDX didn't forbid that, and rollup-plugin-typescript2 didn't forbid that either, so I don't know what you're referring to.

We must have leverage tsconfig configuration that cannot be overwritten, but at the same time, we need to give the opportunity to expand our presets. And this is the main point.

The solution lies only in the design of the new API.

Yea you can add a tsconfigConcat option for this, and that would be a "new API". Breaking the current API by re-inventing the entire definition of default creates a whole set of new problems and new unintuitive behavior. Except while a deep merge may be unintuitive for arrays, it is still correct behavior. A concatenation is simply wrong behavior, because that's not what default means.

A tsconfigConcat would solve your use-case and is more intuitive because it literally has concat in the name and that is exactly what it does.
Changing tsconfigDefaults to mean "concatenation" is not because that's, for the fifth time, not what default means.

default =\= concatenation. That is not my opinion, that is the definition.

[agilgur5]

Well I didn't take words out of people's mouths, not read their actual words, make things up, attack the character of a contributor, provide 0 links, give 0 alternatives, and only consider a single option and no others, that was you.

If you'd like to attack a contributor that is just trying to be helpful and is giving real definitions, real use-cases, and real alternatives, I guess that's your perogative 🤷

[ezolenko]

I think I was under impression that lodash merge would in fact merge arrays (interleave them or something) when I was writing that part. Or I wasn't even paying attention to what happens to arrays. Since it apparently overwrites contents of arrays using indexes as element names (right? is this javascript thing about arrays originally not being true arrays, but instead being dictionaries that happened to have numbers for keys?), our documentation at the moment lies.

Ideally, default/main/override tsconfigs options should allow for customization of all parameters, both scalar values and arrays, in the same unsurprising way. Lodash way of replacing array elements based on index has one major drawback -- you can't create literal sparse arrays (maybe padding array with bunch of undefineds?). That means rollup config must be built dynamically, and tsconfig, being a json file, can't do them at all.

Even with literal sparse arrays, replacing elements based on index is way too fragile.

On the other hand, concatenating is surprising, if nothing else usually behaves this way. And alone doesn't allow removal of previous values.

Switching between concatenating and replacing as I proposed originally is doubly surprising.

Changing both merges to replace arrays wholesale as @agilgur5 proposed is probably a least surprising option, it will allow removing values, at the expense of having to repeat values one wants to keep. It is still a breaking change though and people will have to rewrite their configs for that (see https://xkcd.com/1172/).

We can either change behavior of those options or create a new set and deprecate existing options with a warning.

(sorry if I missed an important point somewhere, I only skimped the whole thread)

[tarsisexistence]

@ezolenko I see the next way

Let's look at the situation and motivation:
The developer wants to add some necessary configurations for typescript via rollup plugin.
In this case, we should not prevent the developer from adding settings safely, and not lose any other settings when merging the configuration and rollup configurations.

What does this tell us? Let's start from semantics:

As I said before

I do agree with default behavior. It makes sense to apply config default only when tsconfig does not have corresponding properties ...

We can redesign this thing completely.

My suggestions are:

1. Apply tsconfigDefaults properties when the original tsconfig does not have corresponding properties.
2. tsconfigOverride should take care of this.
   2.1 It should work as before with scalar values.
   2.2 It should concatenate array properties. (probably this should not break anything and must meet configuration motivation)

I just trying to avoid the third additional rpt2 option, because it will complicate this API by at least 50% and any developer will cease to understand this.

[ezolenko]

@maktarsis how in your approach would dev be able to remove include or exclude entries from tsconfig? The scenario is: you have a tsconfig.json that is used elsewhere, you want to use it in rollup without touching json itself, but you want to remove a line from excludes array for some reason.

[tarsisexistence]

@ezolenko I understood your though.

Naturally, as discussed above, this will not be possible.
But we are talking about possibilities rather than motivation.

I still do not see situations when we need to rewrite the exclude.
Nevertheless, I am of the opinion that we do not need to overwrite dev's "exclude". In other words, I can’t imagine why.
It is necessary to understand the reason for "for some reason".

If you want to have this possibility in any case, then you need to provide an additional API for concatenation.
But as you might understand, I think that it might be unclaimed and simply unnecessary.
The exclusion of something from "exclude" may not be necessary at all, but if we go the other way, then we complicate the understanding of the configuration.

[ezolenko]

We need both, adding something to array that is not there and removing something from it. With shallow merge both adding and removing are done by replicating whole array (with changes) at higher level. Not very convenient, but usable.

[agilgur5]

Noting here that shallow merge has been previously mentioned in this repo a few times: #86 (which mentions TS's shallow merging behavior as I did here), #72 (comment), and #208 (comment)

[agilgur5]

The incorrect reference to concatenation in the docs that I wrote of above has now been fixed by #314.

As such, what remains of this issue duplicates older issues as mentioned in my previous comment, namely #86. The eventual change will be to implement #86 and use shallow merges on arrays (arrays just get replaced, effectively).
That reflects how tsc and tsconfig extends work, so this plugin should behave similarly to those for the principle of least surprise and better ecosystem compatibility.

[agilgur5]

Seems like this also duplicated #196, where both OP and ezolenko stated that the issue was due to _.merge

[agilgur5]

#39 seems to be the original issue that everything duplicates. #39 (comment) mentions extends behavior as well

[agilgur5]

#53 is the original issue from which tsconfigDefaults was implemented, and it pretty explicitly backs up the intent of defaults as being overridden (i.e. my entire point in this thread):

However if the user explicitly does not want declaration files to be generated, we should respect that.

It was also written to support microbundle's usage, and I was describing TSDX's usage in detail as a rebuttal to OP's proposal, which is basically a fork of microbundle.
(I was solo-maintaining TSDX at the time of this issue and while I was not the creator and the creator did not preserve commit history, it was evident that portions of the code directly copied microbundle).

Think that is a fairly decisive nail in the coffin here.