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.

Sign up for GitHub

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

Jump to bottom

Split into create-modular-react-app and modular-scripts packages #29

Merged

sebinsua merged 1 commit into master from create-modular-app

Jul 14, 2020

Contributor

sebinsua commented Jul 13, 2020 •

edited

Loading

Split into create-modular-react-app and modular-scripts packages.

Also, generate the widgets map at transpilation time.

sebinsua commented

View reviewed changes

package.json Outdated

@@ @@ -9,7 +9,8 @@ @@
                 "scripts": {
                   "prettier": "prettier --write .",
                   "lint": "eslint --cache --fix --ext .js,.ts,.tsx --max-warnings 0 .",
-                  "test": "jest"
+                  "test": "jest",
+                  "build": "workspaces-run --ignore eslint-config-modular -- yarn build"

Contributor Author

sebinsua Jul 13, 2020

Yet again I run into that problem where yarn workspaces run doesn't have a way of excluding packages in v1.

Contributor

threepointone Jul 13, 2020

I don't think we need to have all this as one command (which we then need to fix by adding yet another dependency). let's just have separate build commands for each package.

sebinsua commented

View reviewed changes

packages/create-modular-app/src/cli.ts Outdated

+                  stdout: process.stdout,
+                  ...opts,
+                });
+              }

Contributor Author

sebinsua Jul 13, 2020

This is now defined in both of the packages...

Contributor

threepointone Jul 13, 2020

that's fine, don't sweat it. we shouldn't be afraid of copy pasted code, especially when it's as low stakes as this

sebinsua commented

View reviewed changes

packages/create-modular-app/src/cli.ts Outdated

+                } catch (err) {
+                  return false;
+                }
+              }

Contributor Author

sebinsua Jul 13, 2020

This is also now defined in both of the packages.

sebinsua force-pushed the create-modular-app branch from f4355c6 to 8b1326f Compare

July 13, 2020 19:14

sebinsua commented

View reviewed changes

packages/create-modular-app/src/cli.ts Outdated

+                if (!name) {
+                  console.error('Please pass a name into `create-modular-app`.');
+                  process.exit(1);
+                }

Contributor Author

sebinsua Jul 13, 2020 •

edited

Loading

I dropped meow from this package but then needed the positional argument which followed create-modular-app to be treated as the application name.

This code is kind of weird because I was worried by all of the different ways it could be executed (e.g. npx create-modular-app, yarn create modular-app, npm init modular-app, and so on...)

Does anybody have a better idea?

Contributor

threepointone Jul 13, 2020

I'm fine with using/recommending just one one (npx) even if we force yarn for the rest.

sebinsua commented

View reviewed changes

packages/create-modular-app/src/cli.ts Outdated

+                  workspaces: ['app', 'widgets/*'],
+                  modular: {},
+                  scripts: {
+                    start: 'modular-scripts start',

Contributor Author

sebinsua Jul 13, 2020

Do you think we should add a add-widget script into this or something like that?

Contributor

threepointone Jul 13, 2020

I don't think so. Should be ok.

sebinsua commented

View reviewed changes

packages/create-modular-app/src/cli.ts Outdated

+                    // TODO: Enable these once published.
+                    // 'modular-scripts',
+                    // TODO: This config needs to be given a different name.
+                    // 'eslint-config-modular',

Contributor Author

sebinsua Jul 13, 2020

This package name already exists. Does anybody have another?

Contributor

threepointone Jul 13, 2020

eslint-config-modular-app

sebinsua commented

View reviewed changes

packages/create-modular-app/src/cli.ts Outdated

+                delete appPackageJson['scripts'];
+                delete appPackageJson['eslintConfig'];
+                /* eslint-enable @typescript-eslint/no-unsafe-member-access */
+                fs.writeJsonSync(appPackageJsonPath, appPackageJson);

Contributor Author

sebinsua Jul 13, 2020

Quite a lot of the code above should belong to a cra-template.

Contributor

threepointone Jul 13, 2020

yeah, let's do that right after this.

sebinsua commented

View reviewed changes

packages/create-modular-app/src/cli.ts Outdated

+              // TODOS
+              // - make sure react/react-dom have the same versions across the repo
+              // fix stdio coloring

Contributor Author

sebinsua Jul 13, 2020

Additionally, is it an issue that CRA prints messages about commands it's made available, which aren't available (e.g. yarn eject). Hiding messages seems like a total mess...

Contributor

threepointone Jul 13, 2020

don't worry about it. remember that we'll fallback to regular CRA after facebook/create-react-app#1333 is resolved (maybe)

sebinsua commented

View reviewed changes

packages/create-modular-app/template/app/widgets.js Outdated

		@@ -0,0 +1,10 @@
		// DO NOT EDIT THIS FILE.
		// eslint-disable-next-line import/no-extraneous-dependencies

Contributor Author

sebinsua Jul 13, 2020 •

edited

Loading

Sorry about this. I tried creating an .eslintignore for the template directory but it had no effect.

sebinsua commented

View reviewed changes

packages/create-modular-app/template/app/widgets.js Outdated

+                const { getModularRoot, generateWidgetMap } = require('modular-scripts');
+                module.exports = generateWidgetMap(path.join(getModularRoot(), 'widgets'));
+              `;

Contributor Author

sebinsua Jul 13, 2020 •

edited

Loading

The other way of doing this would be: with the separate modular map command or via a webpack plugin.

I think the separate command made the scripts code more complicated because map was run during other commands to keep the code and directories in sync.

While the special wepback virtuals modules plugin is that it's webpack-specific.

This makes the source of truth clearer, however the con is that it's showing people 'how the sausage is made'.

Contributor

threepointone Jul 13, 2020

does this work with babel's caching? add a widget, and see if the map updates. I'm not sure it will, unless this issue's been fixed recently.

If that doesn't work, I think manually generating might still be the right thing to do for now. This should probably have been in a separate PR.

Contributor

threepointone Jul 13, 2020

Further, assuming we're going to start recommending typescript (or some static type system) as the default, how would this work?

Strongly leaning towards keeping the manual method for longer than we'd think.

Contributor Author

sebinsua Jul 13, 2020 •

edited

Loading

I remember the caching problem you're talking about and it's still not fixed, however ...it seems to be working.

I am not sure why though? It could be because of the yarn add ... or the call to yarn prettier. I guess it would be bugged, if somebody mutated the file-system without using modular-scripts add [widget-name]? Deal-breaker? Update: I tried without those, and we need some form of yarn install, to make the packages available in the workspace, however it appears to regenerate that file no matter what. I have no idea why it's working.

I noticed that the cache issue has this comment against it: > **Note**: This issue is not present when used in Create React App. This refers to this comment.

Here's the fix.

I don't really like that there is a command which people need to know about in addition to the command to add a widget, but on the other hand I'm not asserting that this is actually a good solution... 👀

If this was rewritten to have static typing, I was going to suggest that this output into a function body, and then set the type with the return type of this. export default getWidgetMap();

sebinsua commented

View reviewed changes

packages/create-modular-app/template/gitignore Outdated

+              # See https://help.github.com/articles/ignoring-files/ for more about ignoring files.
+              # dependencies
+              node_modules

Contributor Author

sebinsua Jul 13, 2020

This isn't the original CRA .gitignore, since that only ignored the root-level /node_modules.

sebinsua commented

View reviewed changes

packages/modular-scripts/src/cli.ts Outdated

+                  description: 'Dashboards for a new generation',
+                  help: `
+                Usage:
+                  $ modular-scripts add <name>

Contributor Author

sebinsua Jul 13, 2020

Is modular-scripts add a good name or does yarn modular-scripts add feel confusingly like yarn add?

Contributor

threepointone Jul 13, 2020

can't we name the bin modular?

Contributor Author

sebinsua Jul 13, 2020 •

edited

Loading

What do others think? I changed the name to be in line with react-scripts, kcd-scripts, etc. But we could have a package called modular-scripts (or even something scoped) which created a binary called modular. That's also OK.

Contributor

threepointone Jul 14, 2020

But we could have a package called modular-scripts (or even something scoped) which created a binary called modular. That's also OK.

yeah this is what I meant. I still think we'll eventually get the package name fwiw.

Contributor

NMinhNguyen Jul 14, 2020

You can have diff bin names and package names, although maybe for a modular bin I'd expect @modular/cli or modular-cli?

sebinsua commented

View reviewed changes

packages/modular-scripts/src/index.ts

		@@ -0,0 +1,2 @@
		export * from './getModularRoot';
		export * from './generateWidgetMap';

Contributor Author

sebinsua Jul 13, 2020

These are now used by the app/src/widgets.js created by create-modular-app.

sebinsua commented

View reviewed changes

packages/modular-scripts/template/widgets/starter/index.test.js

               test('it should render', () => {
                 const el = document.createElement('div');
-                expect(() => render(<Component$$ />, el)).not.toThrow();
+                expect(() => render(<ComponentName$$ />, el)).not.toThrow();

Contributor Author

sebinsua Jul 13, 2020

This was a bug within my last PR.

sebinsua force-pushed the create-modular-app branch from 8b1326f to 07f26f4 Compare

July 13, 2020 19:33

sebinsua requested review from NMinhNguyen and susanafcosta

July 13, 2020 20:07

sebinsua force-pushed the create-modular-app branch from 07f26f4 to d22d57b Compare

July 14, 2020 11:19

threepointone approved these changes

View reviewed changes

Contributor

threepointone left a comment

stamping

sebinsua force-pushed the create-modular-app branch from d22d57b to 7ed2117 Compare

July 14, 2020 11:51

sebinsua changed the title ~~Split into create-modular-app and modular-scripts packages~~ Split into create-modular-react-app and modular-scripts packages

sebinsua force-pushed the create-modular-app branch from 7ed2117 to bc1df24 Compare

July 14, 2020 11:52


          Split into create-modular-react-app and modular-scripts packages

34c8853

Also, generate the widgets map at transpilation time.

sebinsua force-pushed the create-modular-app branch from bc1df24 to 34c8853 Compare

July 14, 2020 11:55

sebinsua merged commit f123294 into master

sebinsua deleted the create-modular-app branch

July 14, 2020 11:56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet