-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add named export #5526
Add named export #5526
Conversation
To avoid issues with default exports like `apollo_server_plugin_response_cache_1.default is not a function`
@DaSchTour: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
Is there a general problem where default exports don't work for you? It's a common pattern in npm packages, and we use it in a few other places in this repo, so just making this one change doesn't make too much sense. How are you writing your import/require such that it doesn't work? |
@glasser I repeatedly had problems with default exports in different projects. I see this as a anti-pattern in JS development. In this case I had the issue when using the |
Funny thing is, that I also have cacheControl set. But that also doesn't seam to do anything. NestJS and Apollo doesn't seam to really play well when it comes to caching 😞 |
@@ -1,3 +1,4 @@ | |||
import plugin from './ApolloServerPluginResponseCache'; | |||
export default plugin; | |||
export { plugin as ApolloServerPluginResponseCache }; | |||
module.exports = plugin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there is something weird here. Why does this file have both export default
and module.exports =
? That's the problem, which I'll fix.
For some reason, these plugins defined their exports both using `export default` and `module.exports =`. In practice this means that the latter overrides the former or something, which in some contexts causes problems (see #5526).
For some reason, these plugins defined their exports both using `export default` and `module.exports =`. In practice this means that the latter overrides the former or something, which in some contexts causes problems (see #5526).
I suspect that #5542 fixes the issue. While I agree that named exports are generally nicer than defaults, I think "you can use either default or a name to get the same thing" is worse than either choice so for these particular packages I'm going to leave them as the defaults. As far as getting caching working goes, are you seeing cache-control response headers set? A post in GH discussions or community.apollographql.com might be a good place to get help with that too. |
For some reason, these plugins defined their exports both using `export default` and `module.exports =`. In practice this means that the latter overrides the former or something, which in some contexts causes problems (see #5526).
I'm not sure if you saw, that it's possible to do very strange things with this default export and it only works with typecasting, because the typings do not match the import. That's very bad developer experience. And this is also one of the few apollo packages that have a default export. |
@DaSchTour If the "very strange things" still occur in |
@glasser following the documentation using 3.1.1 I get this error
|
This isn't about named exports. You have two copies of |
To avoid issues with default exports like
apollo_server_plugin_response_cache_1.default is not a function