Added Jest test to verify UMD API-forwarding for scheduling package #13532
Conversation
|
Seems good but I'm concerned these UMDs don't follow the convention. I understand why (there's no separate versions) but I'd prefer if they matched the naming scheme so we have the freedom to actually embed the implementation into them later without changing the paths. If we do this, we'd need to change the test to compare both dev and prod bundles (even if they're the same). |
|
Sure. Sounds reasonable. |
Details of bundled changes.Comparing: 46950a3...3a33a3d react-scheduler
Generated by 🚫 dangerJS |
…sts to cover those also.
|
The future consideration was a good catch. Thanks for bringing that up. I've copied the two files and renamed them to match our standard pattern. Also updated tests to cover them both, and updated the fixtures file. |
| const umdAPIProd = require('../../npm/umd/react-scheduler-tracking.production.min'); | ||
| const cjsAPI = require('../../tracking'); | ||
| const secretAPI = require('react/src/ReactSharedInternals').default; | ||
| compareAPIS([umdAPIDev, umdAPIProd, cjsAPI, secretAPI.SchedulerTracking]); |
There was a problem hiding this comment.
I feel kind of clever about this test even though it's silly 😁
| @@ -16,25 +16,26 @@ describe('Scheduling UMD bundle', () => { | |||
| jest.resetModules(); | |||
| }); | |||
|
|
|||
| function compareAPIS(apis) { | |||
There was a problem hiding this comment.
Nit: maybe APIs 😄 (I don't care really)
| function compareAPIS(apis) { | ||
| apis = apis.map(api => Object.keys(api).sort()); | ||
| for (let i = 1; i < apis.length; i++) { | ||
| expect(apis[0]).toEqual(apis[i]); |
There was a problem hiding this comment.
Might be a bit clearer if the "source of truth" API is passed as first argument. Then you can remove the let i = 1 special case. Also errors would be better if you do
expect(apis[i]).toEqual(nodeAPI);because it's UMD/Secrets API that are actual and the original source code that is expected.
|
Urrghhh GitHub shows my fresh comments as outdated whyyyy |
|
Ran all test permutations and Flow checks locally since CircleCI/Yarn are still busted. |
|
@bvaughn Did you see my comments? |
|
Yikes. no, I didn't see any comments at all. Which ones did I miss? |
|
Let me repost lol |
| function compareAPIS(apis) { | ||
| apis = apis.map(api => Object.keys(api).sort()); | ||
| for (let i = 1; i < apis.length; i++) { | ||
| expect(apis[0]).toEqual(apis[i]); |
There was a problem hiding this comment.
Might be a bit clearer if the "source of truth" API is passed as first argument. Then you can remove the let i = 1 special case. Also errors would be better if you do
expect(apis[i]).toEqual(nodeAPI);because it's UMD/Secrets API that are actual and the original source code that is expected.
|
Yuck! They just popped in! |
|
Why is everything half broken today 😅 I will do another commit here in a sec. |
|
(By the way this is why I sometimes just use "add comment" instead of "review". I just don't have trust GitHub won't insta-hide them..) |
|
I think we should undo the prod/dev split. The mental model here is that Webpack should ideally include the scheduler in its runtime for everyone. So it needs to be tiny and way unopinionated. |
|
Should we undo it for CJS too? Should there be an ESM version? I think we'll need a more detailed plan there. After this change it's consistent with other React packages. If we want to make it sufficiently different let's first decide what's the intended layout structure and what UMD bundle will contain. Otherwise it's neither here nor there. |
|
I added a bullet to our Tuesday sync to decide on these specific things ^ Let's figure it out then 😄 |
…acebook#13532) * Added Jest test to verify UMD API-forwarding for scheduling package * Added separate dev/prod UMD bundles for scheduler package
Additional automated check to ensure the two APIs stay in-sync.