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

Move @types/express to devDeps & type-check the .d.ts file #5

Merged
merged 3 commits into from
Oct 23, 2017
Merged

Move @types/express to devDeps & type-check the .d.ts file #5

merged 3 commits into from
Oct 23, 2017

Conversation

toverux
Copy link
Contributor

@toverux toverux commented Oct 5, 2017

Three little commits, three little things :

  • Add express as a devDep. npm test was failing on my machine because it's used in test.js without being mentioned in package.json.
  • Move @types/express from deps to devDeps, as discussed in @types/express should be moved under devDependencies #4. Works fine if the client has @types/express in it's own dependencies, which should be the case. Otherwise, that user isn't working in strict mode and having or not @types/express won't make any difference.
  • Add a little command to check the syntax and validity of the .d.ts file. This will avoid the risk of breaking that file unintentionally.

@LionC
Copy link
Owner

LionC commented Oct 16, 2017

Move @types/express from deps to devDeps, as discussed in #4. Works fine if the client has @types/express in it's own dependencies, which should be the case. Otherwise, that user isn't working in strict mode and having or not @types/express won't make any difference.

Can you clarify that? I think I do not fully understand the consequences you are describing.

@toverux
Copy link
Contributor Author

toverux commented Oct 16, 2017

I think that there are two main cases:

  • The user uses TypeScript normally. He/she has likely installed express before express-basic-auth, and @types/express alongside because otherwise he/she couldn't have used express in the first place. And then that user adds express-basic-auth. As the definitions for express are already there (installed as a direct dependency), boom, compiles.

  • The user has enabled a variety of flags / cheated the TypeScript compiler to make possible the import of files without type definitions. Then express-basic-auth removes @types/express as a dependency. TypeScript can't find the declaration file for express. Nobody cares as it's allowed. User just looses intellisense on express-specific symbols.

Worst case scenario: TypeScript fails, giving a clear and unambiguous message saying that @types/express should be installed.

@LionC LionC merged commit f9a1ad1 into LionC:master Oct 23, 2017
@LionC
Copy link
Owner

LionC commented Oct 23, 2017

Merged, thank you

@vjpr
Copy link

vjpr commented Feb 2, 2019

@toverux

He/she has likely installed express before express-basic-auth, and @types/express alongside because otherwise he/she couldn't have used express in the first place

This is a bad assumption. To be valid, a pkg must explicitly define its dependencies. Otherwise you are relying on hoisting. This will break pnpm and Yarn PnP.

@toverux
Copy link
Contributor Author

toverux commented Feb 3, 2019

Yes, but TypeScript was a special case. Lots of people were doing that, and maybe still do. Indeed, we were relying on hoisting since it was a known behaviour of all the package managers at the time.
Maybe yarn pnp is a bit young if it's still so fragile and intolerant? :)
According to you, we should put @types/express in dependencies? How are other packages doing today?

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

Successfully merging this pull request may close these issues.

None yet

3 participants