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

Broken types - Types are not been exported correctly (not part of dependencies) #2008

Closed
diervo opened this issue Feb 23, 2018 · 9 comments · Fixed by #2108
Closed

Broken types - Types are not been exported correctly (not part of dependencies) #2008

diervo opened this issue Feb 23, 2018 · 9 comments · Fixed by #2108

Comments

@diervo
Copy link
Contributor

diervo commented Feb 23, 2018

When running tsc in a package that contains rollup, tsc blows up due to missing type dependencies from rollup.

Here is one example of the missing dependencies:

error TS7016: Could not find a declaration file for module 'source-map'. '/Users/dval/Sites/raptor/node_modules/source-map/source-map.js' implicitly has an 'any' type.
  Try `npm install @types/source-map` if it exists or add a new declaration (.d.ts) file containing `declare module 'source-map';`

The reason is because rollup exposes the types, and those types depend on other package types that are not included as dependencies.

For example: Module.ts require types from package source-maps, but that package is not part of the npm dependencies (in fact rollup has no dependencies at all).
https://github.com/rollup/rollup/blob/master/src/Module.ts#L17

The same happens for acorn and chokidar.

The solution would be to move those packages from devDependencies, to dependencies.

I will happily do a PR is my assumptions are correct ( I'm not a typescript expert :) )

@lukastaegert
Copy link
Member

Sorry for the inconvenience here, this is an unfortunate side-effect of our recent switch to TypeScript. However including these dependencies is not what we want for rollup as they are already bundled into rollup in a much more efficient way and thus completely unnecessary for non-TypeScript users. In fact they are not even necessary for TypeScript users as none of these are user facing types, we would just need to convince the TS compiler.

You are also not the first to suggest this: #1965 (review). Any idea how to solve this without forcing unneeded dependencies upon users is very welcome!

@diervo
Copy link
Contributor Author

diervo commented Feb 23, 2018

Maybe @rbuckton will be able to help here?

@aluanhaddad
Copy link

aluanhaddad commented Feb 24, 2018

Personally, it makes sense for them to be dependencies. The download cost for users not employing them is trivial and will not affect the behavior of the library.

Currently flattening declarations between packages is a hard problem that has yet to be solved.

That the distribution of Rollup does not depend on the JavaScript of the corresponding packages, does not imply that it should not depend on their types as it is actually rather common in TypeScript for modules to export types alone.

Currently, any TypeScript project that has a direct or transitive dependency on Rollup will receive errors.

I have read the other related issues and pull requests and seen the workarounds, but they are not discoverable (especially as a project may have no direct dependency on Rollup), and I don't think the alternative of running with --skipLibCheck for this reason is at all acceptable.

@guybedford
Copy link
Contributor

Can confirm this is super annoying. Marking as a bug!

@lukastaegert
Copy link
Member

So you agree we should add the dependencies?

@aluanhaddad
Copy link

Relatedly, dependencies that ship declarations as part of their package, such as magic-string, instead of relying on a separate @types package, also produce errors. This is a tricky question because, as far as I understand, Rollup philosophically does not wish to push these source dependencies onto its consumers.

@guybedford
Copy link
Contributor

As I've stated before I'd be interested to explore if we could manually maintain an interface file for Rollup, treating the public API surface area of Rollup as something very carefully managed. This file could then be imported into the internal types and used internally to avoid duplication of interfaces.

Alternatively, yes, adding these runtime dependencies for types is the way to do it.

@lukastaegert
Copy link
Member

This file could then be imported into the internal types and used internally to avoid duplication of interfaces

Sounds like the best approach for me, too. For now, I would add the dependencies to the next major release and then we can explore this however we see fit.

@laughinghan
Copy link

laughinghan commented Mar 28, 2018

Which specific types are being proposed to be moved from devDependencies to dependencies?Just @types/source-maps?

I'm finding that having @types/acorn in dependencies is causing an error in projects that target ES3, and proposing moving it to devDependencies: #2094

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants