Skip to content

Conversation

@merlinnot
Copy link
Contributor

Description

Whereas @types/express is actually a dependency, as it's types are re-exported, the other three are only used internally - they are therefore not included and used in built package - an easy find in lib can confirm that. I moved it to devDependencies, so it's not installed together with the package, making installation more performant - which is important especially in FaaS environments.

Code sample

Not relevant.

@merlinnot
Copy link
Contributor Author

@thechenky

Copy link
Contributor

@thechenky thechenky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what broke our v3.0.0 release - these types need to be in dependencies.

@merlinnot
Copy link
Contributor Author

Are you sure all of these are required, not only @types/express? It would make perfect sense, as I described in the PR description.

I just tested it locally, doing npm pack, initialising an empty project, linking it locally, compiling - all worked just fine.

@thechenky
Copy link
Contributor

Ah yes, you're right, I didn't notice that @types/express was still under dependencies. I think that's the only one we need. I just cloned your fork to confirm this. When I remove @types/express I get compile errors, same as what we saw when releasing v3.0.0, but with it back in, no errors. Going to run integration tests now.

@thechenky
Copy link
Contributor

Oh actually one nit - the ordering is a bit off in the package.json - chai should be above chai-as-promised and there's a couple others.

@thechenky
Copy link
Contributor

Integration tests passed. This is good to go in!

@thechenky thechenky merged commit 6b0d234 into firebase:master Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants