Skip to content

Revert "Fall back to other methods of schema loading... (#533)#613

Merged
martijnwalraven merged 1 commit intomasterfrom
abernix/revert-pr-533
Oct 23, 2018
Merged

Revert "Fall back to other methods of schema loading... (#533)#613
martijnwalraven merged 1 commit intomasterfrom
abernix/revert-pr-533

Conversation

@abernix
Copy link
Copy Markdown
Member

@abernix abernix commented Oct 4, 2018

This reverts commit b48e7bf.

As corroborated by the trending number of 👍 emotes on #560, this PR introduced a number of try catches which silenced relatively useful information from their tried operations. I'm happy to help figure out a better approach but since this seems to have caused many more problems than it solved, we should consider a different approach.

cc @shadaj

Closes #560

@abernix abernix requested a review from shadaj October 4, 2018 08:03
{ url: dependency.schema },
config.projectFolder
);
} catch {}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Throwing away all errors which occur from the fetchSchema method buries not only just the mere failure to acquire the schema in some manner, but it also neglects to reveal very important information about HTTP transport errors, file system errors, GraphQL execution and more — all of which happen via fetchSchema!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Faced this yesterday, because of this catch - SSL certificate error was swallowed.

@shadaj
Copy link
Copy Markdown
Contributor

shadaj commented Oct 4, 2018

This makes sense given the difficulty of tracing the root error. Maybe a future change could handle this better by aggregating the individual errors when no available sources work for the returned error? But this looks good otherwise!

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.

4 participants