Skip to content
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

Move composite-checkout-wpcom into client/lib #37729

Closed
wants to merge 1 commit into from

Conversation

nbloomf
Copy link
Contributor

@nbloomf nbloomf commented Nov 19, 2019

Changes proposed in this Pull Request

Moving this package into calypso so we get hot reloading during development. It may or may not make sense to move some of this out of calypso later, particularly the parts that are tightly bound to the billing backend.

This also sets the Jest test environment locally to ensure we use jsdom, per the unit testing readme.

@nbloomf nbloomf requested a review from sirbrillig November 19, 2019 18:36
@nbloomf nbloomf requested a review from a team as a code owner November 19, 2019 18:36
@matticbot
Copy link
Contributor

@nbloomf nbloomf force-pushed the move/composite-checkout-to-calypso branch from 08fbe7e to 9ff92a3 Compare November 19, 2019 18:56
@sirbrillig
Copy link
Member

This test is failing because composite-checkout uses @wordpress/data which uses @wordpress/priority-queue which does not test for the existence of the window object before using it (eg: typeof window === undefined) and therefore will only work in a context where the window object exists. That object is part of the Web API, so it's only present in browsers, and presumably whatever test runner is being used to launch this test suite is running the javascript without a browser environment (unlike the test runner we're using in composite-checkout which uses jsdom).

So there are a few things we could try. The first thing IMO would be to look into the source code for @wordpress/priority-queue and submit a PR that tests for window before using it. On the other hand, it might be better or easier to see how the tests added (moved) in this PR are being run and modify the runner to use react-testing-library instead.

Moving this package into calypso so we get hot reloading during development. It may or may not make sense to move some of this out of calypso later, particularly the parts that are tightly bound to the billing backend.
@nbloomf nbloomf force-pushed the move/composite-checkout-to-calypso branch from 9ff92a3 to 09a11d3 Compare November 19, 2019 20:17
@sirbrillig
Copy link
Member

sirbrillig commented Nov 19, 2019

The demo webpack config still references /packages/, so the demo does not work.

@sirbrillig
Copy link
Member

sirbrillig commented Nov 19, 2019

Is it necessary to have a webpack config apart from the demo? Is it used? How about the TS config? I'm not really sure how this directory gets used as a library. We can probably remove the package.json too, right? Unless maybe we do need it... Do we need an index.js?

@sirbrillig
Copy link
Member

🤔 My suggestion is to remove the demo entirely, since we can use the actual page in Calypso for that purpose. That simplifies the lib a fair amount by removing the need for any webpack config, package.json, typescript config, demo directory, and (I think) jest config.

@blowery
Copy link
Contributor

blowery commented Nov 20, 2019

I think we can get hot-reloading working without moving the package if that helps... We just need to add a watcher for packages/** and have it run npm run build-packages when it sees a change. Once the rebuild happens, webpack's watcher should pick it up and redo the webpack piece.

@blowery
Copy link
Contributor

blowery commented Nov 20, 2019

on the window checks, you might find WordPress/gutenberg#16227 of use or interesting. Need to finish pushing that forward.

@nbloomf
Copy link
Contributor Author

nbloomf commented Nov 20, 2019

will only work in a context where the window object exists.

Just to close the loop on this, issue #22637 is about this very problem, and links to the resolution in the test docs; it's possible to set the test environment (node by default, which doesn't have window) on a per-file basis.

@nbloomf
Copy link
Contributor Author

nbloomf commented Nov 21, 2019

This has been rendered obsolete by #37802.

@nbloomf nbloomf closed this Nov 21, 2019
@nbloomf nbloomf deleted the move/composite-checkout-to-calypso branch November 21, 2019 16:21
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