Skip to content
This repository was archived by the owner on Jun 28, 2023. It is now read-only.

feat: remove @synhaptein fork from falcor-http-datasource#1

Closed
ardeois wants to merge 2 commits into
masterfrom
remove-forked-dependencies
Closed

feat: remove @synhaptein fork from falcor-http-datasource#1
ardeois wants to merge 2 commits into
masterfrom
remove-forked-dependencies

Conversation

@ardeois
Copy link
Copy Markdown
Contributor

@ardeois ardeois commented Jul 4, 2017

This was used to have dynamic headers, but this could be handled by
client directly. If you want dynamic headers, you could have an object
with dynamic properties.
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty for more details

@ardeois
Copy link
Copy Markdown
Contributor Author

ardeois commented Jul 4, 2017

This is a breaking change!

Copy link
Copy Markdown

@ludovicthomas ludovicthomas left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM. Not sure we had unit tests on that?

@ardeois
Copy link
Copy Markdown
Contributor Author

ardeois commented Jul 5, 2017

No we don't unfortunately

ardeois added 2 commits August 1, 2017 15:16
This was used to have dynamic headers, but this could be handled by
client directly. If you want dynamic headers, you could have an object
with dynamic properties.
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty for more details

BREAKING CHANGE: `dynamicHeaders` is not supported anymore, use `headers` config instead
@ardeois ardeois force-pushed the remove-forked-dependencies branch from 6743fd3 to bd0add2 Compare August 1, 2017 19:18
@ardeois
Copy link
Copy Markdown
Contributor Author

ardeois commented Aug 1, 2017

@synhaptein @ludovicthomas Version updated, ready to merge and publish a new major version

@ludovicthomas
Copy link
Copy Markdown

I just checked the diff, and it's ok for the dynamic headers, but we are loosing the 'status' property on failed that was retrieved from an existing fork (see Netflix/falcor-http-datasource@master...ajoslin:status-code-build).

For more details on our current diff with master, please check Netflix/falcor-http-datasource@master...Mixgenius:landr_custom.

Copy link
Copy Markdown
Contributor

@synhaptein synhaptein left a comment

Choose a reason for hiding this comment

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

LGTM

@ardeois
Copy link
Copy Markdown
Contributor Author

ardeois commented Aug 2, 2017

Oh right good catch @ludovicthomas . This seems to be related to a discussion with @synhaptein yesterday, where removing falcor-router fork means we'll loose the status as well (for GET).
Maybe it would be better if we start by this (only if we find a creative alternative without forking)?

@synhaptein
Copy link
Copy Markdown
Contributor

arfff... Netflix/falcor-http-datasource#33 has never been merged :/

Copy link
Copy Markdown
Contributor

@synhaptein synhaptein left a comment

Choose a reason for hiding this comment

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

Thx to @ludovicthomas, I totally missed the merged I did from a PR still pending on the netflix repo :/ sorry for that

@ardeois
Copy link
Copy Markdown
Contributor Author

ardeois commented Aug 8, 2017

Closing this PR until I find a solution for this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants