-
Notifications
You must be signed in to change notification settings - Fork 36
feat: base plugin repack #29
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
9013470 to
531e8b5
Compare
097224b to
ad48ebf
Compare
| } else { | ||
| fs.copyFileSync(srcFile, distFile); | ||
| // merge package.json files | ||
| if (nodePath.basename(srcFile) === '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.
[nit]
- would be good to extract to separate function e.g.
mergePackageJson. - Should also check for duplicated dep names, if the version is the same, then ok, but in case of mismatch should warn (or try to resolve to common subset?)
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.
Check 83e22d3
| @@ -0,0 +1,9 @@ | |||
| { | |||
| "name": "rnef-plugin-platform-ios-template", | |||
| "scripts": { | |||
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.
💜
| @@ -0,0 +1,9 @@ | |||
| { | |||
| "name": "rnef-plugin-platform-ios-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.
We can omit name as it's getting ignored 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.
Nx started to weirdly fail without the name, like it would consider this package.json as a project's one. After trying a few things and removing all the caches, I went with this solution.
packages/create-app/src/lib/fs.ts
Outdated
| } | ||
| const dist = JSON.parse(fs.readFileSync(to, 'utf-8')); | ||
| dist.scripts = { ...dist.scripts, ...src.scripts }; | ||
| dist.devDependencies = removeDuplicateDependencies({ |
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 merge also regular deps & peer deps
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.
For now we're not doing this so I opted out. Happy to introduce this logic once necessary.
| fs.copyFileSync(from, to); | ||
| } | ||
| const dist = JSON.parse(fs.readFileSync(to, 'utf-8')); | ||
| dist.scripts = { ...dist.scripts, ...src.scripts }; |
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] we should check for script overwrite too
packages/create-app/src/lib/fs.ts
Outdated
| { devDependencies: {} } | ||
| ); | ||
|
|
||
| return { |
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.
Warn if there are duplicate deps entries with version mismatch?
| (api: PluginApi): PluginOutput => { | ||
| api.registerCommand({ | ||
| name: 'dev', | ||
| name: 'start', |
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 we're back to React Native convention?
start → for starting development server?
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 don't have strong opinions about it. We can change anytime
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 guess we wanted to have more web-like experience, but it might create some unpleasant change for rnc cli users, when we proceed it with new convention.
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, on the other hand I thought we'd create our own dev server and hook metro/re.pack there and that's apparently not necessary at the moment. happy to revisit that
f0f312b to
a370651
Compare
c4d38eb to
abc3e4f
Compare

Summary
Got Metro to work, so had to get base Re.Pack support 🚀.
On top I've added logic to merge package.json files (and adjusted other plugins to follow), repurposed
rewritePackageJsontosortDevDepsInPackageJsonand added a helper for making dev deps unique in dist package.json.Test plan
Need to add some later
rnef.config.mjs: