Create a yeoman generator for React+Typescript components #1548

bryceosterhaus · 2019-02-13T01:13:58Z

Our previous generator was named generator-metal-clay so I renamed it and removed the metal reference and then added the react and ts support.

wincent · 2019-02-13T08:38:01Z

packages/generator-clay-component/app/templates/_package.json

+		"compile": "tsc",
+		"prepublish": "npm run compile",
+		"start": "webpack-dev-server --mode development",
+		"test": "jest"


We should probably build a ".d.ts" file as well (something like I am doing here in my test side-project).

matuzalemsteles · 2019-02-13T19:36:24Z

packages/generator-clay-component/app/templates/_README.md

+
+## Contribute
+
+We'd love to get contributions from you! Please, check our [Contributing Guidelines](CONTRIBUTING.md) to see how you can help us improve.


We can update the CONTRIBUTING.md link.

Suggested change

We'd love to get contributions from you! Please, check our [Contributing Guidelines](CONTRIBUTING.md) to see how you can help us improve.

We'd love to get contributions from you! Please, check our [Contributing Guidelines](https://github.com/liferay/clay/blob/master/CONTRIBUTING.md) to see how you can help us improve.

bryceosterhaus · 2019-02-14T22:34:21Z

@wincent @matuzalemsteles

I updated the structure a bit and moved some of the dependencies around. I couldn't find any great examples of this sort of monorepo that both the benefits of a monorepo and also the individual autonomy of the package. So I moved the testing infrastructure up to the root of the project and then also provided shared configs at the root.

@wincent I didn't fully copy your example you provided because I thought it required too much interdependency between packages for building. Reason I avoided that is because packages could technically be independent of other packages in this repo. Such as clay-badge, the only dependency would be on clay-css which isn't a build dependency.

Feel free to test it out by using the generator from the root of the repo.

I'm open to more suggestions in changing this around though. I'm not totally satisfied with this solution yet.

bryceosterhaus · 2019-02-14T23:26:18Z

Decided to make additional changes... I wasn't quite happy with the last change.

Changes:

Hoisted jest/ts/babel configurations to root level
Each package has its own dependencies
Each package has its own build/test scripts. These can be run by...
- cd ./packages/some-clay-component && yarn test
- yarn workspace @clayui/come-clay-component run test
- yarn test from root. This runs lerna run test which recursively runs that script in each module.

This way each module is autonomous in development, but also can be developed in parallel to other components with lerna.

IMO this is the way that seems to make most sense to me.

Let me know if you guys have any concerns or thoughts

wincent

This is looking really great @bryceosterhaus. I left a few comments because I think it is important to get this as close to perfect as possible before we open the floodgates and start generating packages with it, but this looks pretty darn close to ready.

wincent · 2019-02-15T07:04:40Z

packages/generator-clay-component/__tests__/test-app.js

+
+let path = require('path');
+let assert = require('yeoman-generator').assert;
+let helpers = require('yeoman-generator').test;


These let look like they should all be const.

wincent · 2019-02-15T07:05:34Z

packages/generator-clay-component/__tests__/test-app.js

+		);
+		assert.fileContent('src/MyComponent.tsx', /export default MyComponent/);
+	});
+});


Just as style feedback, I think in tests we should favor arrow functions over function () {} for brevity, and value of this is irrelevant here.

wincent · 2019-02-15T07:07:43Z

packages/generator-clay-component/__tests__/test-app.js

+		]);
+	});
+
+	it('content of MyComponent.tsx', function() {


This should read grammatically as an assertion; instead of "it content of MyComponent.tsx" it should be something like: "it produces MyComponent.tsx with templated content" (for example).

Suggested change

it('content of MyComponent.tsx', function() {

it('produces MyComponent.tsx with templated content', function() {

wincent · 2019-02-15T07:09:18Z

packages/generator-clay-component/app/index.js

+let _ = require('lodash');
+let chalk = require('chalk');
+let yeoman = require('yeoman-generator');
+let yosay = require('yosay');


More let to const here, I think.

wincent · 2019-02-15T07:11:50Z

packages/generator-clay-component/app/index.js

+let yosay = require('yosay');
+
+module.exports = yeoman.generators.Base.extend({
+	initializing: function() {


Can we assume this will be running in the context of a recent enough Node version that we can use ES6 object concise methods? ie.

module.exports = yeoman.generators.Base.extend({ initializing() { // etc...

wincent · 2019-02-15T07:21:56Z

packages/generator-clay-component/app/templates/_package.json

+		"webpack-cli": "^3.1.2",
+		"webpack-dev-server": "^3.1.10",
+		"webpack": "^4.29.0"
+	},


Pretty much all dev dependencies can and I would argue should be hoisted up to the repo root. The node modules resolution algorithm means that require('foo') will still be found when working inside a package directory. Rationale is that we ensure that we are using a standard set of dev dependencies across all packages without any divergent versions. I know that you want developers to be able to work within package directories, but they still need to clone the repo in order to do that, so declaring the dev dependencies at the top level doesn't make working locally any harder. Take a look at a typical monorepo project like Babel and you'll see that all the dev dependencies are at the top, with only a couple of exceptions.

If you're interested, I have a checker in that sample project that checks to make sure that dev dependencies only get declared at the top. Other checks are for things like missing deps, mismatched deps etc.

I had tried that originally but then I ran into issues with resolutions when I was in a package itself. Something couldve been out of wack so I will try that again though.

I'd say it's worth it. Babel at least has some docs on set-up for monorepos, although that might not help with the resolution issues you were having.

wincent · 2019-02-15T07:24:03Z

packages/generator-clay-component/app/templates/src/_Boilerplate.tsx

+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+import * as React from 'react';
+import getCN from 'classnames';


Maybe we should bind this to classNames, which is what the "classnames" README examples use.

wincent · 2019-02-15T07:24:49Z

packages/generator-clay-component/app/templates/src/tests/_Boilerplate.tsx

+import * as TestRenderer from 'react-test-renderer';
+import <%= componentName %> from '../<%= componentName %>';
+
+describe('<%= componentName %>', function() {


Suggested change

describe('<%= componentName %>', function() {

describe('<%= componentName %>', () => {

I see you're already using arrow functions in the it examples.

wincent · 2019-02-15T07:25:57Z

packages/generator-clay-component/app/templates/src/tests/_Boilerplate.tsx

+import <%= componentName %> from '../<%= componentName %>';
+
+describe('<%= componentName %>', function() {
+	it('should render', () => {


Style: generally "should" should be avoided in example descriptions. You can always express it more concisely by leading directly with a verb. This is useful because it makes it more likely that you can fit the description in a single line. So in this case it would be:

Suggested change

it('should render', () => {

it('renders', () => {

wincent · 2019-02-15T07:26:49Z

tsconfig.json

+		"removeComments": true,
+		"baseUrl": "./",
+		"paths": {
+			"@clay/*": ["packages/*/src"],


Suggested change

"@clay/*": ["packages/*/src"],

"@clayui/*": ["packages/*/src"],

To match the change you made elsewhere? AFAIK we've requested @clay but no idea whether that can/will happen.

matuzalemsteles

hey @bryceosterhaus, I left some comments. I know we can improve the way we package each package, I had opened an issue about it in #1107. But I think we can improve this in the future.

matuzalemsteles · 2019-02-15T14:26:59Z

package.json

 		"lerna": "lerna bootstrap -- --no-optional --no-package-lock",
 		"link": "lerna run link",
 		"lint": "eslint packages/**/*.js examples/**/*.js scripts/**/*.js",
 		"pa11y": "pa11y-ci ./packages/clay-*/demos/a11y.html",
 		"prettier": "prettier-eslint packages/**/**/*.js examples/**/**/*.js scripts/**/**/*.js",
-		"start": "http-server . -p 4000",


hey @bryceosterhaus, I know we had talked about the demos and that this would not be the best option, I've been thinking about testing https://storybook.js.org/, I've seen some use cases in a monorepo, I think it is a great experience. We can test it once we start having some components in the master.

Yeah I agree we definitely need easily accessible demos on the repo level. Storybook is pretty great, I have used it in the past and it was easy to use. One other thing I am investigating is the possibility of using "Framer X" and that could also possible be a way to run demos. I am working with some of the UX leads here in LA and gonna determine if framer fits our needs.

I think this can be a tracked in a differ issue though, so I have created one #1562

Oh great, I tested a component of Metal.js at the time with Framer X worked up, even though it was not React, I built a layer to give compatibility... The big problem was the CSS that are scoped.

matuzalemsteles · 2019-02-15T14:38:57Z

package.json

 		"lerna": "lerna bootstrap -- --no-optional --no-package-lock",
 		"link": "lerna run link",
 		"lint": "eslint packages/**/*.js examples/**/*.js scripts/**/*.js",
 		"pa11y": "pa11y-ci ./packages/clay-*/demos/a11y.html",
 		"prettier": "prettier-eslint packages/**/**/*.js examples/**/**/*.js scripts/**/**/*.js",
-		"start": "http-server . -p 4000",
-		"test": "npm run build && npm run jest && npm run pa11y",
+		"test": "lerna run test && npm run pa11y",


Just putting my thoughts about it:

One of the points I like about having the jest running at the root is that we will only have one jest startup running and we can reap the coverage much better than terms in each package its coverage... This facilitates the information on https://coveralls.io/github/liferay/clay.

We will have many components here still, so running the test for each project will take a long time for you to know if you have not broken another component that depends on it.

When running the tests in this way, we will have the high boot time in each package, since we raise the jest all the time to test the package, maybe increase the CI time.

I like to run the tests at the root because I can select which tests I want to run when I'm working on just one project. yarn jest packages/clay-badge for example.

Ah yeah that makes sense. I think if we hoist devDependencies like Greg had suggested then we can also probably run a single instance of Jest at both the root and package levels. Ill look into this.

bryceosterhaus · 2019-02-15T19:53:40Z

@matuzalemsteles @wincent just updated.

Here are the highlights

Moved devDependencies to root. (we can still call the individual package scripts by using yarn build in the package itself)
Refactored some things from review comments
Cleaned up package.json dependencies at root
Created clay-badge component via the generator to showcase use

Also I added charts to the eslint and prettier ignore for now. Gonna re-write it to use typescript and ill remove those from the ignore files in that issue.

matuzalemsteles · 2019-02-15T20:54:02Z

packages/clay-badge/package.json

@@ -0,0 +1,31 @@
+{
+	"name": "@clayui/clay-badge",


hey @bryceosterhaus as we now have the scope, I think it is no longer necessary to add clay-, just leaving @clayui/badge.

.eslintrc

wincent · 2019-02-18T09:22:27Z

packages/generator-clay-component/app/templates/_package.json

+	"scripts": {
+		"build": "babel src --root-mode upward --out-dir lib --extensions .ts,.tsx",
+		"build:types": "tsc --project ./tsconfig.declarations.json",
+		"prepublish": "npm run build && npm run build:types",


This should probably be "prepublishOnly", right?

https://docs.npmjs.com/misc/scripts

bryceosterhaus · 2019-02-19T22:51:58Z

@matuzalemsteles @wincent made updates in regards to your comments, re-ran the generator for clay-badge and then also added ts parser for eslint.

wincent · 2019-02-25T07:58:12Z

CC @marcoscv-work and @carloslancha in case you haven't seen this yet.

wincent · 2019-02-25T08:18:19Z

.eslintrc

 			}
-		]
+		],
+		"sort-vars": 2


A lot of this custom stuff would ideally be merged up into the "eslint-config-liferay" package, but that is currently a little hard for us to update because it is not in the Liferay org repo even though that's where the package metadata says it should be; I have an internal ticket open for sorting that out: https://issues.liferay.com/browse/IFI-482.

Great idea. I had just copied over some of the rules we used in Analytics cloud. We slowly added them as we worked on the project and found them beneficial.

We're in the process of migrating the config to its proper location so we can start adapting it... one thing I'm worried about is if these long list makes sense for all cases or just for the jsx one... should we have different config bases, like eslint-config-liferay and eslint-config-liferay-jsx?

One config to rule them all...

wincent · 2019-02-25T08:19:56Z

package.json

-		"start": "http-server . -p 4000",
-		"test": "npm run build && npm run jest && npm run pa11y",
+		"lint": "eslint \"**/*.{js,ts,tsx}\"",
+		"prettier": "prettier-eslint \"**/*.{js,ts,tsx}\"",


You can safely use single quotes here if you want to avoid the backslash escaping.

wincent · 2019-02-25T08:27:04Z

packages/clay-badge/demo/App.tsx

+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+import * as React from 'react';
+import * as ReactDOM from 'react-dom';


I don't think we need to do the * as thing here (or in the other files either). If you search, you'll find a few discussions about what React exports (and what it "should export"); this is the current reality:

https://github.com/facebook/react/blob/1d6b1660a29c452e954d053a14c5adc42ccf94ba/packages/react-dom/index.js#L14-L16

https://github.com/facebook/react/blob/1d6b1660a29c452e954d053a14c5adc42ccf94ba/packages/react-dom/index.js#L14-L16

And the official docs (ReactDOM, React) just tell you to import React, import ReactDOM.

The reason I use * as React was due to the ts compiler complaining.

I looked into it and we would have to add this option to our tsconfig, "allowSyntheticDefaultImports": true. Not sure if that is the route we want to go though.

I was reading these issues about it, microsoft/TypeScript#3337 and microsoft/TypeScript-React-Starter#8

Any preference on it?

Ah, that's a bummer. I wanted to stick to the pattern in the React docs but allowSyntheticDefaultImports seems like a bit of a hack. So * as React is probably the least horrible option.

Yeah thats what I figured as well. Ill just leave it as is for now, and then if for some reason TS fixes this, we can make the change everywhere.

wincent · 2019-02-25T08:29:00Z

packages/clay-badge/src/__tests__/__snapshots__/ClayBadge.tsx.snap

@@ -0,0 +1 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP


I would have expected there to be some content in here.

Updated this in the generator

wincent · 2019-02-25T08:29:59Z

packages/clay-badge/webpack.config.js

+const HWP = require('html-webpack-plugin');
+
+module.exports = {
+	entry: path.join(__dirname, './demo/App.tsx'),


nit: if you're using path.join, you don't need the "./" in "./demo/App.tsx".

Updated this in the generator

.eslintrc

bryceosterhaus · 2019-02-26T18:05:58Z

Once the generator portion of this PR is good to go, I am going to re-generate the badge component based on the generator. I'll also squash all of the generator commits into a single commit.

matuzalemsteles · 2019-02-26T20:12:35Z

Just started reviewing :)

Sent from GH.

packages/generator-clay-component/app/templates/_webpack.config.js

matuzalemsteles

LGTM!

matuzalemsteles · 2019-02-27T17:03:25Z

hey guys, I think we're ready to go with that forward, @wincent do you have anything else you want to comment on? I want to merge this so we can go ahead with #1440 and #1427 and apply the patterns we've added here.

wincent · 2019-02-28T08:09:47Z

Nah, I don't have anything else. I think this is a good enough base upon which we can iterate further. I wanted to make sure we didn't rush this, in order to give people time to comment, but I figure that at this point everybody who cares has probably already shared their opinions.

jbalsas · 2019-02-28T08:14:00Z

I care and I haven't! (I'll just take a quick look right away, but I'm sure it'll be to my liking 🤓)

.travis.yml

packages/clay-badge/src/ClayBadge.tsx

wincent · 2019-03-01T21:51:52Z

Ah, a thing I forgot to mention. .eslintrc is deprecated; we should use .eslintrc.js instead.

bryceosterhaus · 2019-03-01T22:43:51Z

Updated

bryceosterhaus · 2019-03-07T18:47:54Z

@jbalsas , Let me know if there is anything else wanted in this PR.

matuzalemsteles · 2019-03-08T17:37:18Z

hey guys, just an update about the @clay scope in npm: I received the response from the NPM team that it will not be possible for them to proceed even though the user did not reply to their emails because there are packages in the user account that are still being downloaded very frequently. I think we'll have to keep @clayui. 😞

cc @wincent, @bryceosterhaus, @jbalsas

bryceosterhaus · 2019-03-08T18:38:00Z

I went ahead and created a new PR with a cleaned up commit history, seen here #1648

jbalsas · 2019-03-12T10:41:38Z

Closing in favour of #1648 to reduce noise here!

bryceosterhaus force-pushed the generator branch from 63e59fc to b1b8927 Compare February 13, 2019 01:27

wincent reviewed Feb 13, 2019

View reviewed changes

wincent mentioned this pull request Feb 13, 2019

clay-badge in React+Typescript #1547

Closed

matuzalemsteles reviewed Feb 13, 2019

View reviewed changes

bryceosterhaus added 5 commits February 14, 2019 14:24

Create a yeoman generator for React+Typescript components

0c475a2

Remove boilerplate component

50f0bb2

Re-organize structure of shared config

12c4480

Remove reference to clay-charts-shared

bd3e149

Regen yarn.lock

abce730

bryceosterhaus force-pushed the generator branch from b1b8927 to abce730 Compare February 14, 2019 22:29

One more refactor

66198ea

wincent reviewed Feb 15, 2019

View reviewed changes

matuzalemsteles reviewed Feb 15, 2019

View reviewed changes

bryceosterhaus added 5 commits February 15, 2019 11:46

Refactor again

7af2233

Are we even using husky?

7404990

Generate yarn.lock

ef1855b

Create clay-badge by using generator

88f79e8

Remove pa11y for now and unused dependency

205c8df

Fix eslintrc

173b846

matuzalemsteles reviewed Feb 15, 2019

View reviewed changes

wincent reviewed Feb 18, 2019

View reviewed changes

.eslintrc Outdated Show resolved Hide resolved

wincent reviewed Feb 18, 2019

View reviewed changes

wincent mentioned this pull request Feb 18, 2019

RFC: Explore using a centralized script runner abstraction #1567

Closed

bryceosterhaus added 2 commits February 19, 2019 14:50

Update

f11ab79

Update eslint parser and clay-badge

ea2f7cc

wincent reviewed Feb 25, 2019

View reviewed changes

Update

29af023

bryceosterhaus mentioned this pull request Feb 25, 2019

Create a yeoman generator for react components #1605

Closed

matuzalemsteles reviewed Feb 26, 2019

View reviewed changes

.eslintrc Outdated Show resolved Hide resolved

Update year to 2019

bd3860e

matuzalemsteles reviewed Feb 26, 2019

View reviewed changes

packages/generator-clay-component/app/templates/_webpack.config.js Show resolved Hide resolved

matuzalemsteles approved these changes Feb 26, 2019

View reviewed changes

jbalsas reviewed Feb 28, 2019

View reviewed changes

.travis.yml Show resolved Hide resolved

jbalsas reviewed Feb 28, 2019

View reviewed changes

packages/clay-badge/src/ClayBadge.tsx Outdated Show resolved Hide resolved

Use yarn for travis CI

cbd0efe

jbalsas reviewed Mar 1, 2019

View reviewed changes

packages/clay-badge/src/ClayBadge.tsx Outdated Show resolved Hide resolved

bryceosterhaus added 2 commits March 1, 2019 14:37

Update props interface for ...otherProps

4e28a0e

Add file extension for eslintrc

241a0d3

wincent mentioned this pull request Mar 2, 2019

Sync with latest changes from liferay/clay liferay/eslint-config-liferay#25

Closed

jbalsas closed this Mar 12, 2019

wincent mentioned this pull request May 27, 2019

Clean up imports and hooks #2022

Merged

wincent mentioned this pull request Dec 16, 2019

TS editor inference and autocomplete doesn't seem to work. #2775

Closed


		## Contribute

		We'd love to get contributions from you! Please, check our [Contributing Guidelines](CONTRIBUTING.md) to see how you can help us improve.

	it('content of MyComponent.tsx', function() {
	it('produces MyComponent.tsx with templated content', function() {

	describe('<%= componentName %>', function() {
	describe('<%= componentName %>', () => {

	"@clay/": ["packages//src"],
	"@clayui/": ["packages//src"],

Create a yeoman generator for React+Typescript components #1548

Create a yeoman generator for React+Typescript components #1548

Conversation

bryceosterhaus commented Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryceosterhaus commented Feb 14, 2019

bryceosterhaus commented Feb 14, 2019

wincent 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matuzalemsteles 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

Choose a reason for hiding this comment

bryceosterhaus commented Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryceosterhaus commented Feb 19, 2019

wincent commented Feb 25, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryceosterhaus commented Feb 26, 2019

matuzalemsteles commented Feb 26, 2019

matuzalemsteles left a comment

Choose a reason for hiding this comment

matuzalemsteles commented Feb 27, 2019

wincent commented Feb 28, 2019

jbalsas commented Feb 28, 2019

wincent commented Mar 1, 2019

bryceosterhaus commented Mar 1, 2019

bryceosterhaus commented Mar 7, 2019

matuzalemsteles commented Mar 8, 2019

bryceosterhaus commented Mar 8, 2019

jbalsas commented Mar 12, 2019

bryceosterhaus commented Feb 13, 2019 •

edited

Loading

bryceosterhaus commented Feb 15, 2019 •

edited

Loading