[v4] Export TestProvider with defaults and fix typescript react issue #1810

AndrewMusgrave · 2019-07-10T16:48:36Z

WHY are these changes introduced?

Apps consuming polaris don't have a way to use our internal contexts

There's a types issue when reacting context

WHAT is this pull request doing?

Adding defaults to test provider
Exporting it
Fix type issues
export theme provider
export internal contexts :( see below
adding a named export to rollup so we can export from test-utilities/react-testing
move within content context to src/utilities
optional strict mode for test provider and default props

AndrewMusgrave · 2019-07-10T16:49:19Z

src/index.ts

 export {UnstyledLinkProps, LinkLikeComponent} from './utilities/link';
 export {createPolarisContext} from './utilities/create-polaris-context';
+
+export {TestProvider} from './test-utilities/react-testing';


Exporting this so consumers can use it

AndrewMusgrave · 2019-07-10T16:49:37Z

src/test-utilities/react-testing.tsx

-  stickyManager,
-  appBridge,
-  link,
+  themeProvider = createThemeContext(),


Adding default context so we don't have to expose our internals

AndrewMusgrave · 2019-07-10T16:50:08Z

src/test-utilities/react-testing.tsx

    appBridge,
    link,
  }) {
-    const polarisContextDefault = createPolarisContext();


Removing this since we don't use it anymore

AndrewMusgrave · 2019-07-10T16:51:19Z

src/utilities/app-bridge/context.tsx

@@ -1,6 +1,6 @@
-import {createContext} from 'react';
+import React from 'react';


React doesn't need to be in scope from a babel perspective but in consuming apps I was getting a typescript reference errors with react so I'm thinking this will fix it 🤞

I wonder if this is because typescript expects React to be in scope in tsx files. As we never use any JSX would renaming these files to .ts fix it too?

I thought that as well, but no 🎲

AndrewMusgrave · 2019-07-11T21:22:13Z

config/rollup/index.js

      }),
-      commonjs(),
+      commonjs({
+        namedExports: {


I had trouble exporting TestProvider because of an issue with createMount. Here I'm following the docs (rollup docs, github rollup docs)

AndrewMusgrave · 2019-07-11T21:23:43Z

src/components/UnstyledLink/UnstyledLink.tsx

  );
 }

-export default React.memo(UnstyledLink);


Unfortunately enzyme is having trouble .find(UnstyledLink). We're not going to have a noticeable performance difference from this so I'm removing it for now till we find the root cause/enzyme fixes it.

AndrewMusgrave · 2019-07-11T21:24:20Z

src/index.ts


 export {UnstyledLinkProps, LinkLikeComponent} from './utilities/link';
 export {createPolarisContext} from './utilities/create-polaris-context';
+export {


These are used in web and we can't get around them for now :(

AndrewMusgrave · 2019-07-11T21:24:58Z

src/test-utilities/react-testing.tsx

 type Options = DeepPartial<ComplexProviders> & Partial<SimpleProviders>;
 type Context = ReturnedContext;
-interface Props extends ReturnedContext {
+interface Props extends Partial<ReturnedContext> {


Exporting the provider, now with defaults so props aren't mandatory

AndrewMusgrave · 2019-07-11T21:33:25Z

src/components/WithinContentContext/context.tsx

@@ -1,5 +0,0 @@
-import React from 'react';


This was actually a move to src/utilities

AndrewMusgrave · 2019-07-11T21:34:14Z

src/test-utilities/react-testing.tsx

      ? React.cloneElement(children, props)
      : children;

+  const Wrapper = strict? React.StrictMode : React.Fragment;


Going to have an optional strict mode so this utility can be used for enzyme tests as well

…from bundle

AndrewMusgrave · 2019-07-11T22:19:27Z

config/rollup/index.js

        exclude: 'node_modules/**',
        runtimeHelpers: true,
      }),
-      commonjs({


Ended up removing this since it didn't do what I originally thought

AndrewMusgrave · 2019-07-11T22:20:10Z

src/test-utilities/TestProvider.tsx

@@ -0,0 +1,68 @@
+import React from 'react';


Broke this apart from test-utilities/react-testing since it's not package specific/export issue

I'm not sure what you mean by this? I'm not clear on the value of having this living in its own file

Sorry I could have been more clear. This was the error

Eventually, I decided to move it into a separate file since TestProvider isn't test platform-specific. It can be used with react-testing, enzyme, react-test-util, etc.

Following pairing: This TestProvider shall be the building block of our testing strategy. It is a generic component that can be used by both enzyme and react-testing's mounting systems.

legacy.tsx shall be updated to use this instead of providing its own version.

createPolarisContext shall be removed from our public API and instead 3rd party test configs (e.g. web) should use this TestProvider component as part of their test configuration, for both enzyme and react-testing setups

Related: Can we have a more specific name for this? PolarisTestContextProvider or something like that? It's not very clear what the identifier "TestProvider" would do if you're in web's test setup files.

Related: Can we have a more specific name for this? PolarisTestContextProvider or something like that? It's not very clear what the identifier "TestProvider" would do if you're in web's test setup files.

Yup that's definitely possible, it's actually similar to how I'm importing it right now

import {..., TestProvider as PolarisTestProvider ,...} from '@shopify/polaris'

I think there's some value in not naming our exports so generically people feel compelled to alias them :)

AndrewMusgrave · 2019-07-11T22:20:35Z

src/test-utilities/types.ts

@@ -0,0 +1,33 @@
+import {ClientApplication} from '@shopify/app-bridge';


Broke out shared types to prevent circular imports

AndrewMusgrave · 2019-07-11T22:23:57Z

Should be ready for 👀

`polaris-styleguide` (with a few breaking changes fixed locally)

yarn type-check ✅
yarn test ✅
yarn server ✅
quick tophat ✅

web

types are good but still, need some works on fixes tests

AndrewMusgrave · 2019-07-11T22:24:23Z

cc/ @alex-page @BPScott

BPScott

Could you go over what our testing story is? Regarding modern vs legacy and what we consider "internal to us testing polaris" and "helpers that we publically expose so other teams can use us".

I know we've got "modern" being using @shopify/react-testing and "legacy" using enzyme but I'm not clear on how a test opts into one or the other and what helpers we expose publically for others to use.

BPScott · 2019-07-11T23:45:55Z

src/utilities/within-content-context.tsx

@@ -0,0 +1,3 @@
+import React from 'react';
+
+export const WithinContentContext = React.createContext<boolean>(false);


You don't need the as TS can infer the type from the argument you pass in

BPScott · 2019-07-11T23:46:32Z

src/test-utilities/tests/react-testing.test.tsx


 describe('TestProvider', () => {
-  it('renders in strict mode', () => {
+  it("doesn't renders in strict mode by default", () => {


Suggested change

it("doesn't renders in strict mode by default", () => {

it("doesn't render in strict mode by default", () => {

BPScott · 2019-07-11T23:50:21Z

src/index.ts

+  ScrollLockManagerContext as __UNSAFE_SECRET_INTERNAL_SCROLL_LOCK_MANAGER_CONTEXT,
+} from './utilities/scroll-lock-manager';
+export {
+  WithinContentContext as __UNSAFE_SECRET_INTERNAL_WITHIN_CONTENT_CONTEXT,


I think we want this to be a public not-scary API for now, so that custom cards can take advantage of it per #1303

We shouldn't really expose contexts. There's a private part of the app, and having this publicly available could harm us in the future. We shouldn't be promoting the use of it. From a design perspective if we decide that WithinContext is a variant that could be opted into we should probably expose a prop, rather than our internals. However keeping the export here will allow it to be used, if needed (which was by forked components in web).

As an alternative name we could do with __UNSTABLE_INTERNAL_WITHIN_CONTENT_CONTEXT. And we should mention that _UNSTABLE is to be used at the consumer's own risk and may receive breaking changes in minor versions

On second thoughts keeping this scarey means its' not strictly speaking public API and we can change it up in a minor version. Let's keep it this way;

(Totally missed your replies when I commented there, and it sounds like I ignored ya, sorry)

You point about not exposing contexts is a good one. Keeping this scary internal name now gives us a chance to come up with a proper design for this later.

Poking about with naming - unstable and unsafe suggest they might become stable later, but it sounds like we might want to go a different route with how we expose contexts. React has __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, so perhaps drop that part of the prefix and have __SECRET_INTERNAL_SCROLL_LOCK_MANAGER_CONTEXT and __SECRET_INTERNAL_WITHIN_CONTENT_CONTEXT?

BPScott · 2019-07-12T00:17:15Z

src/test-utilities/TestProvider.tsx

@@ -0,0 +1,68 @@
+import React from 'react';


I'm not sure what you mean by this? I'm not clear on the value of having this living in its own file

AndrewMusgrave · 2019-07-12T21:02:43Z

src/components/ContextualSaveBar/tests/ContextualSaveBar.test.tsx

 import {mount} from 'enzyme';
 import {mountWithAppProvider} from 'test-utilities/legacy';
 import ContextualSaveBar from '../ContextualSaveBar';
-import {FrameContext, createFrameContext} from '../../Frame';


FrameProvider is inside TestProvider so we just need to pass our spies through

AndrewMusgrave · 2019-07-12T21:03:48Z

src/components/DropZone/components/FileUpload/tests/FileUpload.test.tsx

    expect(fileUpload.find(TextStyle).text()).toBe('or drop files to upload');
  });
-
-  it('does not use default action title and hint when props are changed', () => {


This test failed in the conversion and I deleted it since we don't need it anymore. When this component was a class we stored this on state and this was a regression test however we don't follow that pattern in our functional component so 🔥

AndrewMusgrave · 2019-07-12T21:04:17Z

src/components/UnstyledLink/UnstyledLink.tsx

 }

-export default UnstyledLink;
+export default React.memo(UnstyledLink);


I managed to get this working in web so I'm sneakily adding it back 😄

AndrewMusgrave · 2019-07-12T21:04:41Z

src/index.ts

 export * from './components';

 export {UnstyledLinkProps, LinkLikeComponent} from './utilities/link';
-export {createPolarisContext} from './utilities/create-polaris-context';


Just as a quick note in a follow-up PR I can probably remove this entirely from polaris

AndrewMusgrave · 2019-07-12T21:07:21Z

@BPScott 👌

BPScott · 2019-07-12T22:00:24Z

src/test-utilities/legacy.tsx

-      </I18nContext.Provider>
-    );
-  }
+  const appBridge: any = app.appBridge;


I also just spotted that the appBridge typing in tests is kinda wonky. I'll try and come up with a plan once this and #1828 are merged.

Yup app bridge is using {} in tests as a mock but isn't actually a valid type 😬

BPScott

This looks amazing.

One petty naming bikeshed around naming our "internal" exports in https://github.com/Shopify/polaris-react/pull/1810/files#r303160285 but other than that , this is good to go.

I think I've got some half-baked ideas for simplifying some of our types in our tests but that's a plan for later.

BPScott · 2019-07-15T07:15:44Z

Something that's been gnawing at me over the past two days: The more I think, the more I'm unsure of adding FrameContext into the TestProvider. Without it the TestProvider is an AppProvider but with the ability to set not-usually-accessible items like the sticky and scroll managers. That symmetry feels very useful as it makes it easy to describe what TestProvider does - it's the same as AppProvider, but for use in tests so you can set mocks (this helps us find a home for TestProvider too, as we could place it in src/AppProvider (this needs a little more thought)). It also makes the "mountWithAppProvider" function name a bit more true as it only adds the contexts provided by AppProvider and nothing else. Adding a value for FrameContext into the mix muddies the water and I'm not convinced the extra cognitive overhead to understand TestProvider as being a bit different from AppProvider is worth the slight extra convenience of all our contexts in one place - especially as 90% of the time you don't care about defining a value for FrameContext, you only care about appProvider based things.

For internal testing, we could leverage react-testing's about-to-land ability to extend configs. to create a mountWithFrame that extends from mountWithContext so we can still have easy config of the Frame context.

What do you think to leaving FrameContext out of TestProvider and undoing the changes to ContextualSaveBar, Loading and Toast? (eventually those files would be updated to use a mountWithFrame function as described above)

AndrewMusgrave · 2019-07-15T15:47:47Z

I would say, the ideal situation for us is to not have to worry about what mount we are using and whether it contains context that might not necessarily be relevant but required to what we're testing. I think the ability to extend createMount is a cool feature but less needed for us because we have the ability to modify our createMount function.

In polaris we might not use components with frame context a lot however web, embedded apps, etc will since they're creating entire pages.

AndrewMusgrave · 2019-07-15T15:53:58Z

Although if we did decide to have multiple mounts in polaris, I would be ok with that. I still think it's nice to have a all in mount mounting function for consumers though so they don't have to worry about what's going on in polaris since it's not really their concern how we handle contexts and which components use it.

BPScott · 2019-07-15T17:31:55Z

I would say, the ideal situation for us is to not have to worry about what mount we are using and whether it contains context that might not necessarily be relevant but required to what we're testing

To me that feels like many tests will have additional overhead that isn't needed. It won't have any noticeable impact on performance but it makes it a little hard to understand what is relevant and needed for a given test. e.g. "In this test for Button it is wrapped in a FrameContext Provider - does it need data from that context?' - the answer is no, but you can't infer that from looking at the test code.

Ok, I'm happy to merge this as is as I've got some follow-up work that needs it, and we can continue to go back and forth on this later :)

BPScott temporarily deployed to polaris-react-pr-1810 July 10, 2019 16:48 Inactive

AndrewMusgrave commented Jul 10, 2019

View reviewed changes

AndrewMusgrave requested a review from alex-page July 10, 2019 16:51

AndrewMusgrave added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Jul 10, 2019

AndrewMusgrave force-pushed the export-test-provider-fix-types branch from 190cc1f to caf5766 Compare July 10, 2019 16:53

BPScott temporarily deployed to polaris-react-pr-1810 July 10, 2019 16:53 Inactive

BPScott temporarily deployed to polaris-react-pr-1810 July 11, 2019 21:19 Inactive

AndrewMusgrave commented Jul 11, 2019

View reviewed changes

AndrewMusgrave force-pushed the export-test-provider-fix-types branch from c3cccf4 to 80135ef Compare July 11, 2019 21:31

BPScott temporarily deployed to polaris-react-pr-1810 July 11, 2019 21:31 Inactive

AndrewMusgrave commented Jul 11, 2019

View reviewed changes

Export TestProvider with defaults and fix typescript react issue

17d8200

AndrewMusgrave force-pushed the export-test-provider-fix-types branch from 80135ef to 17d8200 Compare July 11, 2019 21:40

BPScott temporarily deployed to polaris-react-pr-1810 July 11, 2019 21:40 Inactive

Split apart TestProvider from react-testing and remove react-testing …

d53e248

…from bundle

BPScott temporarily deployed to polaris-react-pr-1810 July 11, 2019 22:18 Inactive

AndrewMusgrave commented Jul 11, 2019

View reviewed changes

BPScott reviewed Jul 12, 2019

View reviewed changes

BPScott temporarily deployed to polaris-react-pr-1810 July 12, 2019 21:00 Inactive

AndrewMusgrave commented Jul 12, 2019

View reviewed changes

AndrewMusgrave requested a review from BPScott July 12, 2019 21:07

AndrewMusgrave force-pushed the export-test-provider-fix-types branch from e97b761 to 32d871f Compare July 12, 2019 21:23

BPScott temporarily deployed to polaris-react-pr-1810 July 12, 2019 21:23 Inactive

BPScott reviewed Jul 12, 2019

View reviewed changes

BPScott approved these changes Jul 12, 2019

View reviewed changes

BPScott mentioned this pull request Jul 12, 2019

[v4] Stop repeating typedefs and use undefined as default values for hooks #1828

Merged

Refactor legacy tests

c62c6df

AndrewMusgrave force-pushed the export-test-provider-fix-types branch from 32d871f to c62c6df Compare July 15, 2019 17:36

BPScott requested a deployment to polaris-react-pr-1810 July 15, 2019 17:36 Abandoned

AndrewMusgrave merged commit a18b90d into version-4.0.0 Jul 15, 2019

AndrewMusgrave deleted the export-test-provider-fix-types branch July 15, 2019 17:44

AndrewMusgrave temporarily deployed to beta July 16, 2019 18:23 Inactive

		@@ -1,6 +1,6 @@
		import {createContext} from 'react';
		import React from 'react';

		@@ -0,0 +1,33 @@
		import {ClientApplication} from '@shopify/app-bridge';

		@@ -0,0 +1,3 @@
		import React from 'react';

		export const WithinContentContext = React.createContext<boolean>(false);

	it("doesn't renders in strict mode by default", () => {
	it("doesn't render in strict mode by default", () => {

[v4] Export TestProvider with defaults and fix typescript react issue #1810

[v4] Export TestProvider with defaults and fix typescript react issue #1810

Uh oh!

Conversation

AndrewMusgrave commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave commented Jul 11, 2019

polaris-styleguide (with a few breaking changes fixed locally)

web

Uh oh!

AndrewMusgrave commented Jul 11, 2019

Uh oh!

BPScott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave commented Jul 10, 2019 •

edited

Loading

AndrewMusgrave Jul 10, 2019 •

edited

Loading

`polaris-styleguide` (with a few breaking changes fixed locally)

BPScott commented Jul 15, 2019 •

edited

Loading

BPScott commented Jul 15, 2019 •

edited

Loading