build(forma-36-react-components): build with tsdx #611

denkristoffer · 2020-10-29T17:36:13Z

Purpose of PR

This PR moves @contentful/forma-36-react-components to a tsdx build setup. To allow this I had to do some subtasks:

Change to SVG components instead of importing through Babel (This can be supported by Rollup as well, but then we have a difference with Storybook, which uses Webpack. Let's discuss this later).
Lint fixes all over the place as tsdx bring its own eslint config, which we cannot really extend.
Changing some directory structures, like moving forma-36-react-components test config into src/ to have it included in the TS rootDir.
We cannot extend the tsdx babel config outside of tsdx, which means that we cannot get jest to use it. I decided the best solution was then to use tsdx's default config as is, because at least it will be used the same everywhere. This means that we cannot choose our own babel modules and have them work in tests!
This currently doesn't build a file per component like the old setup, so imports like import { CardActions } from '@contentful/forma-36-react-components/dist/components/Card/CardActions/CardActions'; don't work. Do we want to support this?

Some issues

eslint doesn't allow shared configs to bring their own modules, who knew! See eslint/eslint#3458 – this is crazy to me. Anyway, that means we'll have either to "reinstall" the modules used by tsdx's eslint config, like eslint-config-prettier, in each package or we can avoid using hoisting. I prefer the first option because it means less duplication.

PR Checklist

I have read the relevant readme.md file(s)
All commits follow our Git commit message convention
Tests are added/updated/not required
Tests are passing
Storybook stories are added/updated/not required
Usage notes are added/updated/not required
Has been tested based on Contentful's browser support
Doesn't contain any sensitive information

netlify · 2020-11-04T08:26:47Z

Deploy preview for forma-36-react-components ready!

Built with commit 2892a94

https://deploy-preview-611--forma-36-react-components.netlify.app

gui-santos

LGTM! Should we merge this before #610 ?

packages/forma-36-react-components/tools/.storybook/main.js

+    //     require.resolve('babel-loader'),
+    //     require.resolve('react-docgen-typescript-loader'),
+    //   ],
+    // });


denkristoffer · 2020-11-04T13:03:07Z

@gui-santos Sorry, it's not ready yet 😬 The tests aren't running, I think it's because of tsdx issues that I hope will work better after Yarn workspaces are in.

gui-santos · 2020-11-04T13:18:22Z

@gui-santos Sorry, it's not ready yet 😬 The tests aren't running, I think it's because of tsdx issues that I hope will work better after Yarn workspaces are in.

Hmmmm I see!

Should we wait to merge the workspaces because of the frozen deployment? (I'm thinking on the case that someone needs to push a hotfix in the webapp and then it would try to get forma in the pipeline)

denkristoffer · 2020-11-09T07:50:44Z

I got the tests running, but they're currently failing because SVGs seem to not be loaded correctly. Same issue as seen here – will look into this more later, but help is welcome.

suevalov · 2020-11-16T09:01:38Z

...ges/forma-36-react-components/src/components/Accordion/__snapshots__/Accordion.test.tsx.snap

          viewBox="0 0 9 18"
-          xmlns="http://www.w3.org/2000/svg"
+          width="1em"


Are these width/height changes expected? I see a lot of them in different svgs and snapshots

They're from the way @svgr/cli generates components based on SVG files. I don't know if it would be preferable to keep the sizes locked with px?

gui-santos

This is great!

gui-santos · 2020-11-18T12:25:56Z

package.json

@@ -8,9 +8,7 @@
      "packages/components/*"
    ],
    "nohoist": [
-      "@contentful/forma-36-fcss/node-sass-import",
-      "**/tsdx",
-      "**/tsdx/**"


gui-santos · 2020-11-18T13:06:33Z

packages/forma-36-react-components/src/components/Icon/svg/ArrowDown.tsx

+import * as React from 'react';
+
+function SvgArrowDown(props: React.SVGProps<SVGSVGElement>) {
+  return (
+    <svg width="1em" height="1em" viewBox="0 0 24 24" {...props}>
+      <path d="M7 10l5 5 5-5z" />
+      <path d="M0 0h24v24H0z" fill="none" />
+    </svg>
+  );
+}
+
+export default SvgArrowDown;


@burakukula look!
This is basically what we need for an icon package

yeah! exactly! 🙂

gui-santos · 2020-11-18T13:11:05Z

Change to SVG components instead of importing through Babel (This can be supported by Rollup as well, but then we have a difference with Storybook, which uses Webpack. Let's discuss this later).

This is perfect! Since we are dating with the idea of making a separate package for icons. Moving them to components is 50% the work already hahahaha

Lint fixes all over the place as tsdx bring its own eslint config, which we cannot really extend.

If it's a good eslint config, I'm ok with that

This currently doesn't build a file per component like the old setup, so imports like import { CardActions } from '@contentful/forma-36-react-components/dist/components/Card/CardActions/CardActions'; don't work. Do we want to support this?

I don't this will be a problem 😄

eslint doesn't allow shared configs to bring their own modules, who knew! See eslint/eslint#3458 – this is crazy to me. Anyway, that means we'll have either to "reinstall" the modules used by tsdx's eslint config, like eslint-config-prettier, in each package or we can avoid using hoisting. I prefer the first option because it means less duplication.

I prefer the first option too 👍

burakukula

Looks amazing! 🎉
Thanks for that contribution and yeah, most of the work for the icon package is done 🤯 I will try take it and move it to the package in the PR proposal in the next couple of days.

mshaaban0

Awesome work 🍪

I'm not sure about width="1em" height="1em" on svgs especially since the viewbox is still in pixels as well. If there's a way to keep them in pixels it would be awesome.

I'm going to hold of #641 until this is merged, and hope conflicts would be nice to me 😏

packages/forma-36-react-components/package.json

mshaaban0 · 2020-11-23T19:05:23Z

packages/forma-36-react-components/package.json

-    "package.json",
-    "dist"
+    "dist",
+    "src"


Do we want to have src files in the npm package?

Tbh I had the same thought. It's the tsdx default, but I'm not 100% sure why they add it.

None of our experience components have it: https://github.com/contentful/experience-packages/blob/master/packages/experience-components/package.json, so I'd not include it here too.

denkristoffer · 2020-11-24T08:37:46Z

@mshaaban0 (and /cc @suevalov): SVGR automatically adds missing width and height attributes when the output mode is icon, which I used in this case. The svgr PR here doesn't have a lot of reasoning for this, but I trust them to know what they're doing. What do you think, can we keep them or should I remove the attributes?

suevalov · 2020-11-24T09:07:09Z

@denkristoffer are width and height attributes present in the output dist of the main branch? If they're the same then I have no objections

suevalov

Let's remove src folder from the built package, but otherwise is 🎖️

This comment has been minimized.

Sign in to view

denkristoffer force-pushed the tsdx branch from 278e0f8 to 30442b2 Compare November 2, 2020 09:28

This comment has been minimized.

Sign in to view

denkristoffer force-pushed the tsdx branch 3 times, most recently from 87dfc02 to 400f49f Compare November 3, 2020 16:18

gui-santos approved these changes Nov 4, 2020

View reviewed changes

packages/forma-36-react-components/tools/.storybook/main.js Outdated

// require.resolve('babel-loader'),

// require.resolve('react-docgen-typescript-loader'),

// ],

// });

This comment was marked as resolved.

Sign in to view

denkristoffer force-pushed the tsdx branch 2 times, most recently from e599909 to 6b9379a Compare November 9, 2020 07:42

denkristoffer force-pushed the tsdx branch 2 times, most recently from 5523158 to 568d012 Compare November 10, 2020 15:44

denkristoffer mentioned this pull request Nov 11, 2020

chore(deps): migrate from enzyme to @testing-library/react #635

Merged

8 tasks

denkristoffer force-pushed the tsdx branch 6 times, most recently from 86972e7 to c255c7a Compare November 16, 2020 07:17

denkristoffer marked this pull request as ready for review November 16, 2020 07:19

denkristoffer requested review from burakukula, m10l and mshaaban0 as code owners November 16, 2020 07:19

denkristoffer force-pushed the tsdx branch from c255c7a to b5be1ce Compare November 16, 2020 07:24

suevalov reviewed Nov 16, 2020

View reviewed changes

denkristoffer requested a review from gui-santos November 16, 2020 09:57

gui-santos approved these changes Nov 18, 2020

View reviewed changes

burakukula approved these changes Nov 20, 2020

View reviewed changes

denkristoffer force-pushed the tsdx branch 2 times, most recently from 0044739 to bafe503 Compare November 23, 2020 09:16

denkristoffer requested a review from suevalov November 23, 2020 13:26

m10l approved these changes Nov 23, 2020

View reviewed changes

mshaaban0 approved these changes Nov 23, 2020

View reviewed changes

suevalov approved these changes Nov 24, 2020

View reviewed changes

build(forma-36-react-components): build with tsdx

2892a94

denkristoffer force-pushed the tsdx branch from bafe503 to 2892a94 Compare November 24, 2020 15:40

denkristoffer merged commit 3a5f5f2 into master Nov 24, 2020

denkristoffer deleted the tsdx branch November 24, 2020 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(forma-36-react-components): build with tsdx #611

build(forma-36-react-components): build with tsdx #611

denkristoffer commented Oct 29, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

netlify bot commented Nov 4, 2020 •

edited

Loading

gui-santos left a comment

This comment was marked as resolved.

denkristoffer commented Nov 4, 2020

gui-santos commented Nov 4, 2020

denkristoffer commented Nov 9, 2020 •

edited

Loading

suevalov Nov 16, 2020

denkristoffer Nov 16, 2020 •

edited

Loading

gui-santos left a comment

gui-santos Nov 18, 2020

gui-santos Nov 18, 2020

burakukula Nov 20, 2020

gui-santos commented Nov 18, 2020

burakukula left a comment

mshaaban0 left a comment

mshaaban0 Nov 23, 2020

suevalov Nov 24, 2020

denkristoffer Nov 24, 2020

suevalov Nov 24, 2020

denkristoffer commented Nov 24, 2020

suevalov commented Nov 24, 2020

suevalov left a comment

build(forma-36-react-components): build with tsdx #611

build(forma-36-react-components): build with tsdx #611

Conversation

denkristoffer commented Oct 29, 2020 • edited Loading

Purpose of PR

Some issues

PR Checklist

This comment has been minimized.

This comment has been minimized.

netlify bot commented Nov 4, 2020 • edited Loading

gui-santos left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

denkristoffer commented Nov 4, 2020

gui-santos commented Nov 4, 2020

denkristoffer commented Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

denkristoffer Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

gui-santos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui-santos commented Nov 18, 2020

burakukula left a comment

Choose a reason for hiding this comment

mshaaban0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denkristoffer commented Nov 24, 2020

suevalov commented Nov 24, 2020

suevalov left a comment

Choose a reason for hiding this comment

denkristoffer commented Oct 29, 2020 •

edited

Loading

netlify bot commented Nov 4, 2020 •

edited

Loading

denkristoffer commented Nov 9, 2020 •

edited

Loading

denkristoffer Nov 16, 2020 •

edited

Loading