STITCH 2497: Remove core dependency on cross-fetch #230
Conversation
adamchel
left a comment
adamchel
left a comment
There was a problem hiding this comment.
Nice work! Had a couple nits, but otherwise this looks great!
packages/browser/core/src/core/internal/net/BrowserFetchStreamTransport.ts
Show resolved
Hide resolved
packages/core/sdk/__tests__/internal/net/StitchRequestClientUnitTests.ts
Show resolved
Hide resolved
| } from "mongodb-stitch-core-sdk"; | ||
|
|
||
| /** @hidden */ | ||
| export default class JestEventSourceEventStream extends BaseEventStream<JestEventSourceEventStream> { |
There was a problem hiding this comment.
[broader discussion] So this works and I know it's what I told you to do, but now this means that the Transports we wrote are not covered at all by our tests unless we do E2E browser, Node.js, and React Native testing.
We aren't going to make changes to the other transports again for a while probably, but it might be a good idea to file a backlog ticket to do some very rudimentary E2E smoke testing with an actual node server, actual react native app, and actual browser (via something like selenium) to make sure the transports properly interact with the network.
There was a problem hiding this comment.
Considering how core of an element the transports are, I think we should add some basic tests to make sure everything still works, before merging this. More can be added later.
There was a problem hiding this comment.
The point is that these transports can't be tested in Jest, so we'd have to create an entire E2E testing strategy, which I feel is out of scope for this ticket, which was meant to fix a bug.
Tyler did do some manual E2E in the browser to make sure the transports still work, and I think it would be good to merge if he did the same for Server and RN
There was a problem hiding this comment.
Ah, fair point. That works for me.
packages/react-native/core/src/core/internal/net/ReactNativeFetchTransport.ts
Show resolved
Hide resolved
packages/react-native/core/src/core/internal/net/ReactNativeFetchTransport.ts
Show resolved
Hide resolved
4 participants
Added in a few quick fixes for alphabetizing the export lists. Furthermore, not sure if we should test to make sure it works properly in react native