Improvements #13

peterhass · 2020-07-10T09:39:44Z

https://favro.com/organization/aee06c3fe6503c804f507dd9/22c630cc02b09f2d6f98b8b3?card=Goo-30301

This reverts commit 98748b8

peterhass · 2020-07-10T14:33:48Z

package-templates/react/package.json

+    "start": "start-storybook -p 3000 --ci --config-dir ../../.storybook",
+    "start:ssr-preview": "SSR=true start-storybook -p 3000 --ci --preview-url=/ssr-iframe --config-dir ../../.storybook",


Idea: Extract to ../../scripts/start-storybook and ../../scripts/start-ssr-storybook

or start from root and serve all storybooks?

peterhass · 2020-07-10T14:34:16Z

packages/components/package.json

+    "start": "start-storybook -p 3000 --ci --config-dir ../../.storybook",
+    "start:ssr-preview": "SSR=true start-storybook -p 3000 --ci --preview-url=/ssr-iframe --config-dir ../../.storybook",


https://github.com/goodhood-eu/goodhood/pull/13/files#r452882427

max-degterev · 2020-07-10T17:43:24Z

.storybook/main.js

@@ -2,9 +2,10 @@ const path = require('path');
 const sassFunctions = require('sass-functions');
 const sass = require('sass');

-const ROOT_PKG_PATH = path.join(__dirname, '../../../');
+const ROOT_PKG_PATH = path.join(__dirname, '../');


will we run storybook from root or from packages? ⚖️ about the same?

Almost yes but I think it would make a difference in load times. SSR-preview is already pretty slow because it has to load all stories of ONE package but we load all stories in all packages it could get worse.

I think it's ok for subpackages to have their own start, lint, build scripts

max-degterev · 2020-07-10T17:46:33Z

.storybook/ssr/src/index.jsx

@@ -23,7 +23,7 @@ const forceServerSideRender = (story) => {

 const api = start(render);

-require(`${PKG_PATH}/.storybook/preview-ssr`);
+require(`root-pkg/.storybook/preview-ssr`);


do same as in core app? @ or @root and @package?

max-degterev · 2020-07-10T17:49:28Z

package-templates/react/.eslintrc.js

@@ -1,7 +1,7 @@
 const path = require('path');


if we placed .eslintrc.js/babel/mocha at the root wouldn't it work without package configs?

Yes it works, I removed the 'extend' part.

But still, we need those custom rules for no-extraneous-dependencies until there is a better solution for that problem.
import-js/eslint-plugin-import#1174

there seem to be several options within the thread you posted, none work? or maybe disable package checking altogether for now?

tried eslint-import-resolver-lerna with little success

Didn't want to do the crazy stuff this user did Enhancement: Monorepos and no-extraneous-dependencies import-js/eslint-plugin-import#1174 (comment) Would rather have subpackage specific eslintrc.js for now

max-degterev · 2020-07-10T17:50:01Z

package-templates/react/package.json

+    "start": "start-storybook -p 3000 --ci --config-dir ../../.storybook",
+    "start:ssr-preview": "SSR=true start-storybook -p 3000 --ci --preview-url=/ssr-iframe --config-dir ../../.storybook",


or start from root and serve all storybooks?

max-degterev · 2020-07-13T12:29:29Z

.storybook/ssr/src/index.jsx

@@ -23,7 +23,7 @@ const forceServerSideRender = (story) => {

 const api = start(render);

-require(`${PKG_PATH}/.storybook/preview-ssr`);
+require(`@root/.storybook/preview-ssr`);


Suggested change

require(`@root/.storybook/preview-ssr`);

require('@root/.storybook/preview-ssr');

max-degterev · 2020-07-13T12:31:03Z

.storybook/ssr/utils.js

+const matchWebpackTest = (file, test) => (
+  test instanceof RegExp
+    ? test.test(file)
+    : test === file


string? perhaps use .includes?

Not sure if this line is actually needed at all. Almost all rules use a regex test (at least the ones we want to target here).

A string test may need a more complex check because file path could be relative or something like that? Not sure?

I suggest: Always return false for strings and add a comment

not sure myself, leave a comment yes

max-degterev · 2020-07-13T12:32:07Z

babel.config.js

@@ -1,3 +1,6 @@
+const PKG_PATH = process.cwd();


not .babelrc?

Looks like .babelrc.js doesn't get resolved.
.babelrc -> json, .babelrc.js -> not working

max-degterev · 2020-07-13T12:32:55Z

mocha.config.js

 module.exports = {
-  require: '@babel/register',
+  require: path.join(__dirname, '.mocha/babel-require.js'),


could we do the .mocha/babel-require.js right in this file and then let it require itself?

Not sure what you mean. Had to extract the whole require part to babel-require.js because I have to pass options to babel. Found no other way to do that

max-degterev · 2020-07-13T12:34:02Z

package-templates/react/.eslintrc.js

@@ -1,7 +1,7 @@
 const path = require('path');


there seem to be several options within the thread you posted, none work? or maybe disable package checking altogether for now?

max-degterev · 2020-07-13T12:34:49Z

package-templates/react/.mocharc.js

@@ -1 +1 @@
-module.exports = require("../../presets/mocha.config.js");
+module.exports = require("../../mocha.config.js");


Suggested change

module.exports = require("../../mocha.config.js");

module.exports = require('../../mocha.config.js');

i think this file shouldn't be needed, any reason to have it?

No, you are right. Should be removed. Thought mocha doesn't have config lookup like babel does but after renaming mocha.config.js to .mocharc.js it worked

max-degterev · 2020-07-13T12:35:24Z

package-templates/react/package.json

-    "start": "start-storybook -p 3000 --ci",
-    "start:ssr-preview": "SSR=true start-storybook -p 3000 --ci --preview-url=/ssr-iframe",
+    "start": "../../scripts/start-storybook",
+    "start:ssr-preview": "../../scripts/start-storybook --ssr-preview",


shouldn't we default to SSR?

No, SSR is much slower.

max-degterev · 2020-07-13T12:36:36Z

packages/components/.mocharc.js

@@ -1 +1 @@
-module.exports = require("../../presets/mocha.config.js");


same as above

max-degterev · 2020-07-13T12:37:11Z

packages/map/.mocharc.js

@@ -1 +1 @@
-module.exports = require("../../presets/mocha.config.js");
+module.exports = require("../../mocha.config.js");


same as above

peterhass added 6 commits July 8, 2020 12:36

Change icon component name to improve IDE support

f1e44c1

Move storybook into root package

0fd6818

Merge remote-tracking branch 'origin/master' into improvements

80d4283

Remove react package template specific storybook folder

c89e89e

WIP svg loading

98748b8

Revert "WIP svg loading"

917e58e

This reverts commit 98748b8

peterhass changed the title ~~Improvements~~ 𝙒𝐈𝑃 Improvements Jul 10, 2020

peterhass added 2 commits July 10, 2020 16:14

Move babel and mocha config to root directory

89d486c

Move eslint config to root directory

c7f6c06

peterhass changed the title ~~𝙒𝐈𝑃 Improvements~~ Improvements Jul 10, 2020

Refactor

b8537fb

peterhass commented Jul 10, 2020

View reviewed changes

peterhass added 2 commits July 10, 2020 16:36

Merge remote-tracking branch 'origin/master' into improvements

a5af3e2

Apply new orchestration changes to maps package

1032edd

max-degterev reviewed Jul 10, 2020

View reviewed changes

peterhass added 5 commits July 13, 2020 10:16

Change root and package alias to be more conventional

c31a943

Clean up eslint config

e4836e4

Extract storybook start script

22b8199

Add support for '@root' alias in eslint

e2ef01c

Add support for '@' and '@root' in package source

a40c802

max-degterev reviewed Jul 13, 2020

View reviewed changes

peterhass added 3 commits July 13, 2020 15:00

Get rid of unnecessary mocha configs

1f44b7d

Change mocha config to be more simpler

913857a

Eslint autofix

aa61a58

max-degterev approved these changes Jul 13, 2020

View reviewed changes

Remove bad string test matching

2038eea

peterhass merged commit 5f0342b into master Jul 13, 2020

peterhass deleted the improvements branch July 13, 2020 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements #13

Improvements #13

peterhass commented Jul 10, 2020

peterhass Jul 10, 2020 •

edited

max-degterev Jul 10, 2020

peterhass Jul 10, 2020

max-degterev Jul 10, 2020

peterhass Jul 13, 2020

max-degterev Jul 10, 2020

max-degterev Jul 10, 2020

peterhass Jul 13, 2020

max-degterev Jul 13, 2020

peterhass Jul 13, 2020 •

edited

max-degterev Jul 10, 2020

max-degterev Jul 13, 2020

max-degterev Jul 13, 2020

peterhass Jul 13, 2020

max-degterev Jul 13, 2020

max-degterev Jul 13, 2020

peterhass Jul 13, 2020

max-degterev Jul 13, 2020

peterhass Jul 13, 2020

max-degterev Jul 13, 2020

max-degterev Jul 13, 2020

peterhass Jul 13, 2020

max-degterev Jul 13, 2020

peterhass Jul 13, 2020

max-degterev Jul 13, 2020

max-degterev Jul 13, 2020

		"start": "start-storybook -p 3000 --ci --config-dir ../../.storybook",
		"start:ssr-preview": "SSR=true start-storybook -p 3000 --ci --preview-url=/ssr-iframe --config-dir ../../.storybook",

	require(`@root/.storybook/preview-ssr`);
	require('@root/.storybook/preview-ssr');

		@@ -1 +1 @@
		module.exports = require("../../presets/mocha.config.js");
		module.exports = require("../../mocha.config.js");

	module.exports = require("../../mocha.config.js");
	module.exports = require('../../mocha.config.js');

Improvements #13

Improvements #13

Conversation

peterhass commented Jul 10, 2020

peterhass Jul 10, 2020 • edited

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

peterhass Jul 13, 2020 • edited

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

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

peterhass Jul 10, 2020 •

edited

peterhass Jul 13, 2020 •

edited