Typings should not be in dependencies.

[ian-r-rose]

This will need to be in 0.32. I think this can cause problems when dependencies that rely on @jupyterlab/apputils also depend on @types/react.

[jasongrout]

Just checking - we're not exporting anything from @types/react?

[ian-r-rose]

No, nor should we, I think. All evidence of TS should be gone from the built JS, I believe. I ran into this conflict when updating some extensions that depend on apputils

[ian-r-rose]

Ooh, maybe you are right, @jasongrout. What do you think about VDomRenderer.render()?

[sccolbert]

It's still a dev dependency. Anyone that wants to consume our typings during build needs to install the react typings too (IMO).

[jasongrout]

I thought the purpose of having the types packages was so that you didn't need to install the typings outside of your dependencies. What exactly is the problem with including the typing package as a dependency?

[sccolbert]

Conflicting versions of the types?

[sccolbert]

In theory, the types should be scoped to our current package, but I think typescript has a problem with multiple global definitions of different versions. Although IIRC the newer releases of TS are better at this.

[jasongrout]

Ian, is that what happened to you?

[ian-r-rose]

Yes, there was one set of React typings from the extension, and one from apputils. They conflicted (regardless of the versions), and deleting the ones distributed by apputils fixed the issue.

Happy to consider other options, but as it stands, extension developers can't rely on both apputils and React.

[ian-r-rose]

More discussion of this question:
microsoft/types-publisher#81
microsoft/TypeScript#9731

[jasongrout]

Would it be enough to change the dependency to ^16.0.0 to cast a wider net to avoid conflicts?

[jasongrout]

For me personally, apputils is too big and pulls in too many unrelated things. For example, the widgets jlab manager depends on it transitively, so now we are depending on all sorts of stuff we don't need or want. So for that reason I'm fine with removing the typing package. Also, this particular typing package declares a global, right? If so, that's another reason to not include it as the possibility of conflicts/problems is much greater.

On the other hand, I see the argument for including it. So I'm probably +0 on merging this PR.

[ian-r-rose]

I was unable to get it to build when more than one @types/react was installed with any variations on the versioning (including ^16.0.0). The only thing that I found worked was removing @types/react in the dependencies for apputils.

[ian-r-rose]

(I also generally agree that apputils is getting pretty large)

[vidartf]

Consider the case of someone writing a lab extension in JS. They should not need to have a dependency for typings.

[saulshanabrook]

Yes, there was one set of React typings from the extension, and one from apputils. They conflicted (regardless of the versions)

@ian-r-rose which extension was this? I would like to investigate. From my experience with yarn versioning, it will only install multiple instances of the same library if there are conflicting dependencies. I assume we want to avoid this with React, that there should only be on version of it, since it has global state.

yarn why react might also be helpful to figure out where the conflicting versions are coming from.

[ian-r-rose]

I would be grateful for a second set of eyes on it @saulshanabrook. I have seen it in multiple extensions, but the simplest one is https://github.com/ian-r-rose/jupyterlab-toc, which has an open PR bumping it to the 0.32 packages.

[vidartf]

I assume we want to avoid this with React, that there should only be on version of it, since it has global state.

Note, we're only talking about the typings package here, not the react package itself.

[ian-r-rose]

Another data point: the extension build works fine using npm, but yarn (and thus jlpm) bring in extra copies of the react types that make tsc fail.

[jasongrout]

1. Does react follow semver? i.e., is the 16.3 typings package backwards compatible with the 16.0 typings package?
2. If so, can we just change the typings range to ^ instead of ~?

[jasongrout]

I was unable to get it to build when more than one @types/react was installed with any variations on the versioning (including ^16.0.0).

I meant changing the apputils react typing to ^16.0.0 - is that what you did?

[ian-r-rose]

No, I did not change the apputils react (note that it is very tricky to try to experiment with these, since extensions only target published packages). Why do you think that changing the react version would make a difference?

[jasongrout]

Apputils is pinned to patch updates. If it is instead pinned to minor updates, it's possible that the dedup will work.

[ian-r-rose]

But again, this is just for the @types dependency.

Edit: misunderstood what you said. I am pretty baffled here, but this appears to be a case where yarn is getting it wrong, and npm is getting it right.

[ian-r-rose]

Another update: adding a resolutions field to the extension package.json can force yarn to only install a single version of something, albeit with some warnings:

"resolutions": {
  "**/types/react": "16.3.9"
}

This is not a fully satisfying solution, but seems to work. I think @jasongrout may be right that using ^16.0.0 in apputils would be more reliable.

[vidartf]

@ian-r-rose No magic with lockfiles interfering?

[ian-r-rose]

@vidartf Not as far as I could tell.

I don't think that the @types/react package follows semantic versioning: instead it tries to match the major+minor versions with React, and then the patch versions can be considered API breaking changes, almost by definition. So yarn is not necessarily wrong to not deduplicate @types/reactif it is listed as a dependency of a @jupyterlab/apputils. But this means that tsc will run into conflicting type definitions. So it seems pretty brittle to me to export the typings as a dependency, as @sccolbert suggests.

[jasongrout]

I don't think that the @types/react package follows semantic versioning: instead it tries to match the major+minor versions with React, and then the patch versions can be considered API breaking changes, almost by definition. So yarn is not necessarily wrong to not deduplicate @types/reactif it is listed as a dependency of a @jupyterlab/apputils. But this means that tsc will run into conflicting type definitions. So it seems pretty brittle to me to export the typings as a dependency, as @sccolbert suggests.

It's even worse than that. Type definitions can be compatible with only certain versions of typescript as well (so we have a 2d compatibility matrix here...). Looking at https://www.npmjs.com/package/@types/react, and the tags, we see that for ts v2.5, you should probably use 16.0.36.

I think in general it's a nice idea to include typings in dependencies. In this case, where apparently this particular typings package may be brittle and apputils itself brings in "too many" dependencies anyway from being a smorgasbord of a package, I'm okay with removing these typings.

But I would also like to see if bumping the apputils typings to ^16.0.0 solves the issue.

[ian-r-rose]

I still get errors using ^16.0.0 for @types/react in @jupyterlab/apputils.

[jasongrout]

Okay, +1 on moving the dependency to a dev dependency in this case, then.

[ian-r-rose]

Thanks for publishing @jasongrout. Unfortunately, this PR seems to have broken the build for extensions that don't include @types/react in their dependencies.

This is a frustrating situation. Extensions with @types/react have version conflicts with this package if it includes them. Extensions without it have missing typings. Package management is hard.