-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
deprecate fragments everywhere #984
Conversation
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.
A few comments!
@@ -249,10 +255,23 @@ export default class ApolloClient { | |||
}) as DeprecatedWatchQueryOptions; | |||
} | |||
|
|||
if (options.fragments && !haveWarnedWatchQuery && process.env.NODE_ENV !== 'production') { | |||
console.warn( | |||
'"fragments" option is deprecated and will be removed in the upcoming versions, ' + |
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.
I'd say "in 0.6" rather than "upcoming versions"
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.
Ok
// deprecation warning flags | ||
let haveWarnedQuery = false; | ||
let haveWarnedWatchQuery = false; | ||
let haveWarnedMutation = false; |
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.
Why separate these? I feel like we could just have one haveWarned
variable, since the warning messages are the same anyway.
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.
Yeah, good point. Probably not worth changing though.
@@ -1358,7 +1358,9 @@ it('should not let errors in observer.next reach the store', (done) => { | |||
// hacky solution that allows us to test whether the warning is printed | |||
const oldWarn = console.warn; | |||
console.warn = (str: string) => { | |||
assert.include(str, 'Warning: fragment with name'); | |||
if (!str.match(/deprecated/)) { |
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.
You could use this utility if you want: https://github.com/apollostack/apollo-client/blob/b2d9a7c8c969e3b03f47e349e8b0702547e20e48/test/util/wrap.ts#L14
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.
Yeah, I decided to take a shortcut here because it affected lots of tests and wasn't easy to see which ones. I intend to remove the line after deprecating fragments. I wouldn't do it for deprecating things in general, but I think it's okay to do it in this instance because fragments affect many more test cases than future deprecations will affect.
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.
The warning is showing up whether or not you're using createFragment
because react-apollo
is using it here https://github.com/apollostack/react-apollo/blob/a23883fd226e0352813b4889cf647964c4c0e0fc/src/parser.ts#L8
It's a little annoying to have a warning suddenly appear in every console when you can't do much about it.
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.
@jasonnathan have you filed an issue on react-apollo? That would be the best way to make sure it gets fixed.
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.
TODO: