-
Notifications
You must be signed in to change notification settings - Fork 67
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.
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
implement CLI commands- app, new, test, build, map #27
Conversation
2c01e23
to
428c410
Compare
packages/cli/cli.js
Outdated
' ', | ||
), | ||
); | ||
shell.mkdir('widgets'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little wary of using widgets
and dashboard
semantics - reminds me of enterprise-y apps. But maybe I'm overthinking it because you can have like iOS widgets that have nothing to do with banks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, open to ideas for what this word should be. I started with 'components', but don't want folks to think that they have to do it for literally every component. this might be a question for matthew.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had big discussions around terminology, I'll share the link with you regarding what we've decided internally.
packages/cli/cli.js
Outdated
map(); | ||
} | ||
|
||
function test() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My worry with using .js
for this is that if these functions are rewritten to be async, ESLint won't tell us we need to add await test()
at the call sites and we'll have runaway promises. Whereas @typescript-eslint
is able to catch that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically we might actually have long running processes (eg, running server). either way, happy to rewrite this in ts once it becomes a little more fleshed out and I start adding tests. didn't want to block this work tho, and it's not consumed by any other modules, so exposed types aren't really important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so exposed types aren't really important.
It's more about the internal types than public types, especially given it's a CLI without a programmatic API. Consumers will be invoking it as a CLI. I'm more worried about us making mistakes when iterating on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy to rewrite this in ts once it becomes a little more fleshed out and I start adding tests.
777cc23
to
755b509
Compare
btw, I've removed the need for PR review before merging. I trust everyone to use their best judgement. |
f3466c1
to
3c2fc68
Compare
This commit adds an implementation for the key cli commands. modular app <name>: creates a new repository <name>, with yarn's workspaces enabled. It then creates a package 'app', as a default create-react-app installation. This is then modified to work with yarn's workspaces. We rewrite the app's webpack config to read from a widgets folder, where we'll host other components. We also copy an implementation for a faux 'store' to save configurations, tuples of [component, props]. modular new <name>: This creates a new 'widget' under widgets/<name>. modular test: runs all tests in the repository. modular build: runs a full build, output in app/build modular map: generates a mapping from widget names -> React.lazy() components for those apps. This will be used in conjunction with the store to dynamically load components as needed. This also sets up eslint and prettier in the repo, and can be called with yarn lint and parn prettier, respectively.
packages/cli/cli.js
Outdated
); | ||
shell.exec('yarn add prettier --dev -W'); | ||
shell.exec( | ||
`yarn add ~/code/work/modular/packages/eslint-config-modular --dev -W`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks obv.
packages/cli/cli.js
Outdated
shell.exec('git init'); // TODO - make an initial commit? | ||
} | ||
|
||
function widget(name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, it wasn't originally clear to me that (1) there can only be one app within the repository (and this had a .git
directory, etc), and (2) the widget command only operates within this folder and crashes otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the app shouldn't have a .git directory? but we should be able to modify this for multiple apps in one repo, I simply hadn't planned for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should have a .git
directory. What I'm saying is that before I ran the commands I had misunderstandings of what they did and where they should be run.
Oh I'd promised to land this PR. I'll clean up and land this by tomorrow evening. |
packages/cli/cli.js
Outdated
workspaces: ['app', 'widgets/*'], | ||
modular: { | ||
app: 'app', | ||
widgets: 'widgets', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even want this to be configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question. maybe not.
packages/cli/cli.js
Outdated
const modularConfig = JSON.parse(fs.readFileSync('./package.json', 'utf8')) | ||
.modular; | ||
if ( | ||
typeof modularConfig.app === 'string' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We fail if there is no modular
key within the package.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open to other ideas to detect "this is a modular
repo", this was just one idea.
@threepointone I made some changes in a separate branch, I can do a PR into this or just merge them directly in? And also, feel free to tell me to not do some of these things, because I got carried away 👀 (hence the commit separation is super bad. I am sorry.) What I did:
Btw, it was kind of a pain to work with, because linking didn't bring across necessary binaries, and I kept forgetting to setup links again. Not sure if you had a better debug process than I...?! Also, I agree with your comments. Happy to look into dropping A few thoughts:
|
replies inline.
Feel free to commandeer/overwrite this PR.
Dope.
Dope.
Dope.
Dope.
It's not urgent, so I'd prioritise anything else at the moment. As long as it's not exposed externally, it shouldn't matter.
I keep going back and forth on this. But I'm certain it has to be done automatically, not manually. It's meant to be used by code that loads component definitions from a database or whatever, and it would be annoying to do it by hand whenever . Either way, let's keep the manual command for now and attack it in a later PR.
Yeah I was just extra enthusiastic when I changed the package keys on the import map. I do this we should pascal case the generated component name tho. So
Dope. This also make us not have to worry about the modular package name (tho I'm still going to try and get it). Thanks very much for this effort, it's looking so much better than my attempt. You the best. |
Merged as-is. I'll do those extra things as separate PRs now. |
"eslint-plugin-jsx-a11y": "^6.3.1", | ||
"eslint-plugin-react": "^7.20.0", | ||
"eslint-plugin-react-hooks": "^4.0.5", | ||
"typescript": "~3.9.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we match https://github.com/facebook/create-react-app/blob/2da5517689b7510ff8d8b0148ce372782cb285d7/packages/eslint-config-react-app/package.json#L17-L27 and specify them as peerDependencies
instead? You wouldn't want dependencies like typescript
to be brought in via an ESLint config package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea. I think this config needs to be redone anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea.
Cool, I'll raise a PR later.
I think this config needs to be redone anyway.
Do you have specific points in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have specific points in mind?
I'm not sure how all of the dependencies are being used by the index.js
. Are they in use?
Also, shouldn't we provide people with TypeScript linting, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are they in use?
Yeah, by CRA. All of those are CRA's peerDependencies
.
Also, shouldn't we provide people with TypeScript linting, etc.
CRA does have some TS linting, yeah.
(This is pretty yucky, but I'm trying to share and land stuff incrementally. )
This commit adds an implementation for the key cli commands.
modular app : creates a new repository , with yarn's
workspaces enabled. It then creates a package 'app', as a default
create-react-app installation. This is then modified to work with yarn's
workspaces. We rewrite the app's webpack config to read from a widgets
folder, where we'll host other components. We also copy an
implementation for a faux 'store' to save configurations, tuples of
[component, props].
modular new : This creates a new 'widget' under widgets/.
modular test: runs all tests in the repository.
modular build: runs a full build, output in app/build
modular map: generates a mapping from widget names -> React.lazy() components
for those apps. This will be used in conjunction with the store to
dynamically load components as needed.
This also sets up eslint and prettier in the repo, and can be called
with yarn lint and yarn prettier, respectively.