-
Notifications
You must be signed in to change notification settings - Fork 36
feat(create-app): add basic functionality #5
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
Conversation
9c7bc65 to
5727022
Compare
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.
Warning
Here file has a _ prefix in the name because pnpm run build fails when there's .eslintrc.js because it treats it as a real ESLint config. For now I didn't implement logic for removing underscore prefix inside files because maybe there's a way to avoid adding such logic and simple ignore files under template-* and don't try resolve them, if not let's implement replacement logic for files with _ underscore 👍
|
To treat any platform as first-class citizen from the start, I'd like to encourage you to experiment with splitting this template into three packages:
|
| 'while', | ||
| ]; | ||
|
|
||
| const reservedNames = ['react', 'react-native', ...javaKeywords]; |
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.
nit: any reserved words in Swift/Objective-C?
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.
Expo has pretty short list of reserved names, but that might be due to CNG: https://github.com/expo/expo/blob/6c19a5234eb352566a98b6e0c4d85154074aedee/packages/create-expo/src/Template.ts#L28
Perhaps we can actually allow user to have these names in JS, but do some transformation to make them safe in iOS/Android native context
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.
Thanks for the PR. Left some comments!
Generally speaking, I am not sure whether we want to copy template or reference React Native one directly instead.
Ideally, I would like to have a template that is:
- typescript enabled
- does not have native files (uses CNG by default, or managed workflow)
However, these components are not ready yet, so I guess it makes sense to not clone existing React Native template (because we will create our own in the future anyway with more opinions), but reference it instead.
|
|
||
| Each template is a separate directory with a `package.json` file. | ||
|
|
||
| The template will be available as a dependency in the project and can be selected using the `--template` flag. |
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.
One particular challenge I always have with templates is how do we make sure we can test them frequently. What I found very useful and future-proof is treating each template as an actual standalone project.
This is what we've been doing in Rise Tools as well as Expo:
Following this approach allows you to easily clone and replace what you want, w/o having to create all that logic for renaming and moving files around.
https://github.com/rise-tools/rise-tools/blob/main/packages/create-rise/src/utils.ts#L12-L37
This tar takes fileTransformer and it is what Expo has been using to replace contents / rename files in a stream, as you copy it, super quickly.
TL;DR
I would suggest doing everything we can to keep every template a standalone package in this repository, so we can treat it as a playground and ensure everything works.
| __dirname, | ||
| // Workaround for getting the template from within the monorepo | ||
| // TODO: implement downloading templates from NPM | ||
| '../../../../../templates', |
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.
nit: To start with, maybe we can use existing React Native template for compatibility and build on top of that? We want some sort of easy migration path for existing users, so having the ability to use community template sounds like a good idea.
Otherwise, with iOS and android folders here, we add another unnecessary layer of complexity at this point that we could otherwise avoid, at least for now.
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 can start with RNC template but we would have to do a "migration" step on top of that:
- it is having references to RN CLI in it's scripts, we would have to replace it with our CLI
- add our configs
- potentially remove some RN CLI stuff
Pros:
- Meta/Community would update template for new RN versions for us
- Faster start
Cons:
- Meta/Community changes could break our tool, as they will not care about our use case
- Less control
| @@ -0,0 +1,4 @@ | |||
| module.exports = { | |||
| root: true, | |||
| extends: '@react-native', | |||
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.
TBD for the future, but let's make sure there's an issue for that. If we want to be prescriptive with custom CLI and inits, we should also have custom ESLint configuration (similar to Expo) with basic and other things in the future too.
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.
Re template in general i've created an issue for discussing the details of how do we want to have it: #9
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.
nit: would be great, for custom template, to have custom icon. Tiny little things.
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.
agree! writing down for the future improvements (and once this project will have a logo lol)
| const NAME_REGEX = /^[$A-Z_][0-9A-Z_$]*$/i; | ||
|
|
||
| // ref: https://docs.oracle.com/javase/tutorial/java/nutsandbolts/_keywords.html | ||
| const javaKeywords = [ |
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.
[nit] Do we really need to ban all Java keywords? Perhaps this is a matter of reviewing "project name" value usage in the template
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.
It saved me few times so I wanted to have this also inside this CLI. Creating a project with Java keyword can cause really hard to debug issues 😅
| __dirname, | ||
| // Workaround for getting the template from within the monorepo | ||
| // TODO: implement downloading templates from NPM | ||
| '../../../../../templates', |
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 can start with RNC template but we would have to do a "migration" step on top of that:
- it is having references to RN CLI in it's scripts, we would have to replace it with our CLI
- add our configs
- potentially remove some RN CLI stuff
Pros:
- Meta/Community would update template for new RN versions for us
- Faster start
Cons:
- Meta/Community changes could break our tool, as they will not care about our use case
- Less control
| @@ -0,0 +1,4 @@ | |||
| module.exports = { | |||
| root: true, | |||
| extends: '@react-native', | |||
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.
Re template in general i've created an issue for discussing the details of how do we want to have it: #9
This reverts commit 583309e.
f2775da to
f5802d9
Compare
|
Went through changes from @mdjastrzebski and all looks good to me, shall we merge? 🤩 |


Summary
Add basic functionality for creating a new project.
Test plan
pnpm run buildnode packages/create-app/dist/src/index.js