-
Notifications
You must be signed in to change notification settings - Fork 975
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
Audit CLI commands #594
Audit CLI commands #594
Conversation
Yes to positionals. I haven't tried this out locally, but based on your examples and screenshots, this is definitely an improvement. I'll go through each of your topics but only respond where I suggest something different. So if I don't mention one of your topics in the following, assume that means I agree and you should proceed as you suggested.
|
A few questions: generators: -f not provided as an alias for --force: For overwriting generators, right now you have to explicitly type If yes, then: we do provide this alias for db: The command's name and its alias are reversed: down, up, save: Do we not want to provide access to the other arguments/options? The other arguments/options I'm talking about: For down/up, the user can technically specify the migration to go down/up to in many ways, but our current implementation is only letting the user go down/up the default: 1 migration at a time. For save, there's these options: The reason I'm asking is in the CLI Commands doc I link to the prisma documentation which says you can do all these things, but then we don't actually let you do all these things, so I'm thinking I should remove those links if we don't plan on adding this functionality. If we do plan on adding it, I can add a note for the time being, something like:
|
generators: -f not provided as an alias for --force: db: down, up, save: |
Update on Since the prisma docs say you can, I'm going to remove links to it for now. I'll add them back when it becomes available. |
This one's almost good to go. Double checking some things. But one more question: db up: there's an option
|
That's an excellent, nuanced question -- with significant ramifications if it were to be changed!! There are many Prisma commands where an error results if the Prisma Client does not already exist. So this is why you see However, you don't always want to generate the Client --> most importantly when you deploy!! Check out this build command in [build]
command = "yarn rw db up --no-db-client && yarn rw build" Misc DocsYou've greatly improved the consistency and behavior across all commands. When the time comes to update the CLI Contributing doc, it would be great to include these as a "design guide" for Redwood Yargs. E.g, all commands and options must have description, how to handle alias, using positional, etc. |
NOTE: unit tests are looking good. But this needs manual testing against Tutorial up through deploy due to scope. On me to do when ready for merging. |
@thedavidprice Thanks for that explanation! Now I have another question. 😅 We don’t define a Does yargs do something I’m not aware of, like “if you prefix a command with ‘no’, it equals false”? |
Yes, that's a Yargs' trick: |
Currently working through the rebase right now. There's some changes to the builder that I'm not sure what I should do with? Like this change to the builder of
Should I normalize this pattern across commands? And in helper the builder's populated via a default arg now:
You can already override the builder by defining it in the command and exporting it there. But that's true of any of the constants. So is the pattern for overriding default behavior to pass params to a yargs-wrapping function, or to do it all in the file and export it from there? Sorry I'm moving pretty slow, WIFi's down and using a hotspot on a tenuous sprint "LTE" to push/pull. Pretty radical. |
^^ I'd say not for now. This seems to be isolated to the scaffold due to the complexity of being a command-of-commands each with respective options. It doesn't seem to get in the way of any of the standardizing work you're doing here, correct? Meaning the positional, etc. are still being implemented and working accordingly.
^^ oh, that's a really nice update by Kim! Re: pattern for overriding default --> it's being done in multiple ways right now, correct? The wrapper definitely makes things much more DRY. But I think it's more apparent what each command is doing when handled in the file. For now, I wouldn't spend too much time making this consistent. Just prioritize making it work. FYI Peter has on his mind a complete overhaul of the generators sooner than later, which would improve the structure and possibly add the capability for them to be extensible. For now, this could be called out via a sentence in the CLI Contributing documentation with an explanation as to what's going on and inconsistent approach. (maybe?) |
@thedavidprice Doesn't at all! Just wondered since some of the work in this PR was about making changes consistent.
This sounds awesome! And I think I fixed what was confusing me before. One thing with the default-builder arg though is I can't add And the last thing holding me back is a failing test saying |
I think the problems here:
Might have to do with how I'm using the builder as a function instead of an object. Looking into it. |
I don't know what you're talking about 'cause all I'm seeing is ✅ 😉 |
That was it! So now the question is
Since Suggestion: what I'm doing to get the test to pass is just adding default args to the
I'm gonna say we just leave the test file as it? Since things are going to get reworked soon anyway? Or should I remove the call to |
I can do those tutorial checks for you? |
^^ Hmm, I don't care as much about the tests passing as the commands working! 😉 I believe the "right" way to do this would be to mock the object. However, setting defaults doesn't really bother me as much as knowing they are still being correctly overridden in the specific command files. Can you confirm that's the case? If we have any questions here otherwise, we could loop in Robert to review.
Um, do you need to ask?? I'd be delighted 😆 Just confirm what you did so we remember when it comes time to cut a new release including all this fanciness. |
I went through the tutorial without any errors, expect for |
I restored the test's original strategy by using the pattern in |
^^ woah, this scares me. Can you give me a bit more context here:
|
I tried both. The error was coming up even with the default install of 0.8.2. Steps to reproduce were just:
Immediately after. For the cli package from this branch, I have to run |
Also tested 2 cases:
Super. Suck.
11:51:44 db | Watching... /Users/price/Repos/xx-delete/api/prisma/schema.prisma
11:51:44 db |
11:51:44 db | ✔ Generated Prisma Client to ./../node_modules/@prisma/client in 151ms
11:51:44 db |
11:51:44 db | /Users/price/Repos/xx-delete/node_modules/@prisma/cli/build/index.js:2
module.exports=function(e,t){"use strict";var r={};function __webpack_require__(t){if(r[t]){return r[t].exports}var n=r[t]={i:t,l:false,exports:{}};e[t].call(n.exports,n,n.exports,__webpack_require__);n.l=true;return n.exports}__webpack_require__.ab=__dirname+"/";function startup(){return __webpack_require__(1280)}t(__webpack_require__);return startup()}({1:function(e,t,r){"use strict";Object.defineProperty(t,"__esModule",{value:true});const n=r(8914);const i=n.default("@prisma/studio-server");t.default=i},2:function(e,t,r){"use strict";Object.defineProperty(t,"__esModule",{value:true});var n=r(5622);var i=r(9250);var o=r(21);var s=r(8356);var a=function(){function Reader(e){this.options=e;this.micromatchOptions=this.getMicromatchOptions();this.entryFilter=new o.default(e,this.micromatchOptions);this.deepFilter=new i.default(e,this.micromatchOptions)}Reader.prototype.getRootDirectory=function(e){return n.resolve(this.options.cwd,e.base)};Reader.prototype.getReaderOptions=function(e){return{basePath:e.base==="."?"":e.base,filter:this.entryFilter.getFilter(e.positive,e.negative),deep:this.deepFilter.getFilter(e.positive,e.negative),sep:"/"}};Reader.prototype.getMicromatchOptions=function(){return{dot:this.options.dot,nobrace:!this.options.brace,noglobstar:!this.options.globstar,noext:!this.options.extension,nocase:!this.options.case,matchBase:this.options.matchBase}};Reader.prototype.transform=function(e){if(this.options.absolute){e.path=s.makeAbsolute(this.options.cwd,e.path)}if(this.options.markDirectories&&e.isDirectory()){e.path+="/"}var t=this.options.stats?e:e.path;if(this.options.transform===null){return t}return this.options.transform(t)};Reader.prototype.isEnoentCodeError=function(e){return e.code==="ENOENT"};return Reader}();t.default=a},7:function(e,t,r){var n=r(5747);var i=r(5622);var o=r(4891);var s=r(4581);var a=r(586);var u=r(9366);var l=n.realpath&&typeof n.realpath.native==="function"?n.realpath.native:n.realpath;var f=function isFile(e,t){n.stat(e,function(e,r){if(!e){return t(null,r.isFile()||r.isFIFO())}if(e.code==="ENOENT"||e.code==="ENOTDIR")return t(null,false);return t(e)})};var c=function isDirectory(e,t){n.stat(e,function(e,r){if(!e){return t(null,r.isDirectory())}if(e.code==="ENOENT"||e.code==="ENOTDIR")return t(null,false);return t(e)})};var p=function realpath(e,t){l(e,function(r,n){if(r&&r.code!=="ENOENT")t(r);else t(null,r?e:n)})};var h=function maybeRealpath(e,t,r,n){if(r&&r.preserveSymlinks===false){e(t,n)}else{n(null,t)}};var d=function getPackageCandidates(e,t,r){var n=s(t,r,e);for(var o=0;o<n.length;o++){n[o]=i.join(n[o],e)}return n};e.exports=function resolve(e,t,r){var s=r;var l=t;if(typeof t==="function"){s=l;l={}}if(typeof e!=="string"){var m=new TypeError("Path must be a string.");return process.nextTick(function(){s(m)})}l=a(e,l);var v=l.isFile||f;var y=l.isDirectory||c;var _=l.readFile||n.readFile;var g=l.realpath||p;var S=l.packageIterator;var E=l.extensions||[".js"];var D=l.basedir||i.dirname(o());var x=l.filename||D;l.paths=l.paths||[];var w=i.resolve(D);h(g,w,l,function(e,t){if(e)s(e);else init(t)});var C;function init(t){if(/^(?:\.\.?(?:\/|$)|\/|([A-Za-z]:)?[\/\\])/.test(e)){C=i.resolve(t,e);if(e==="."||e===".."||e.slice(-1)==="/")C+="/";if(/\/$/.test(e)&&C===t){loadAsDirectory(C,l.package,onfile)}else loadAsFile(C,l.package,onfile)}else if(u(e)){return s(null,e)}else loadNodeModules(e,t,function(t,r,n){if(t)s(t);else if(r){return h(g,r,l,function(e,t){if(e){s(e)}else{s(null,t,n)}})}else{var i=new Error("Cannot find module '"+e+"' from '"+x+"'");i.code="MODULE_NOT_FOUND";s(i)}})}function onfile(t,r,n){if(t)s(t);else if(r)s(null,r,n);else loadAsDirectory(C,function(t,r,n){if(t)s(t);else if(r){h(g,r,l,function(e,t){if(e){s(e)}else{s(null,t,n)}})}else{var i=new Error("Cannot find module '"+e+"' from '"+x+"'");i.code="MODULE_NOT_FOUND";s(i)}})}function loadAsFile(e,t,r){var n=t;var o=r;if(typeof n==="function"){o=n;n=undefined}var s=[""].concat(E);load(s,e,n);function load(e,t,r){if(e.length===0)return o(null,undefined,r);var n=t+e[0];var s=r;if(s)onpkg(null,s);else loadpkg(i.dirname(n),onpkg);function onpkg(r,a,u){s=a;if(r)return o(r);if(u&&s&&l.pathFilter){var f=i.relative(u,n);var c=f.slice(0,f.length-e[0].length);var p=l.pathFilter(s,t,c);if(p)return load([""].concat(E.slice()),i.resolve(u,p),s)}v(n,onex)}function onex(r,i){if(r)return o(r);if(i)return o(null,n,s);load(e.slice(1),t,s)}}}function loadpkg(e,t){if(e===""||e==="/")return t(null);if(process.platform==="win32"&&/^\w:[\/\\]*$/.test(e)){return t(null)}if(/[\/\\]node_modules[\/\\]*$/.test(e))return t(null);h(g,e,l,function(r,n){if(r)return loadpkg(i.dirname(e),t);var o=i.join(n,"package.json");v(o,function(r,n){if(!n)return loadpkg(i.dirname(e),t);_(o,function(r,n){if(r)t(r);try{var i=JSON.parse(n)}catch(e){}if(i&&l.packageFilter){i=l.packageFilter(i,o)}t(null,i,e)})})})}function loadAsDirectory(e,t,r){var n=r;var o=t;if(typeof o==="function"){n=o;o=l.package}h(g,e,l,function(t,r){if(t)return n(t);var s=i.join(r,"package.json");v(s,function(t,r){if(t)return n(t);if(!r)return loadAsFile(i.join(e,"index"),o,n);_(s,function(t,r){if(t)return n(t);try{var o=JSON.parse(r)}catch(e){}if(o&&l.packageFilter){o=l.packageFilter(o,s)}if(o&&o.main){if(typeof o.main!=="string"){var a=new TypeError("package “"+o.name+"” `main` must be a string");a.code="INVALID_PACKAGE_MAIN";return n(a)}if(o.main==="."||o.main==="./"){o.main="index"}loadAsFile(i.resolve(e,o.main),o,function(t,r,o){if(t)return n(t);if(r)return n(null,r,o);if(!o)return loadAsFile(i.join(e,"index"),o,n);var s=i.resolve(e,o.main);loadAsDirectory(s,o,function(t,r,o){if(t)return n(t);if(r)return n(null,r,o);loadAsFile(i.join(e,"index"),o,n)})});return}loadAsFile(i.join(e,"/index"),o,n)})})})}function processDirs(e,t){if(t.length===0)return e(null,undefined);var r=t[0];y(i.dirname(r),isdir);function isdir(n,i){if(n)return e(n);if(!i)return processDirs(e,t.slice(1));loadAsFile(r,l.package,onfile)}function onfile(t,n,i){if(t)return e(t);if(n)return e(null,n,i);loadAsDirectory(r,l.package,ondir)}function ondir(r,n,i){if(r)return e(r);if(n)return e(null,n,i);processDirs(e,t.slice(1))}}function loadNodeModules(e,t,r){var n=function(){return d(e,t,l)};processDirs(r,S?S(e,t,n,l):n())}}},21:function(e,t,r){"use strict";Object.defineProperty(t,"__esModule",{value:true});var n=r(8356);var i=r(3386);var o=function(){function EntryFilter(e,t){this.options=e;this.micromatchOptions=t;this.index=new Map}EntryFilter.prototype.getFilter=function(e,t){var r=this;var n=i.convertPatternsToRe(e,this.micromatchOptions);var o=i.convertPatternsToRe(t,this.micromatchOptions);return function(e){return r.filter(e,n,o)}};EntryFilter.prototype.filter=function(e,t,r){if(this.options.unique){if(this.isDuplicateEntry(e)){return false}this.createIndexRecord(e)}if(this.onlyFileFilter(e)||this.onlyDirectoryFilter(e)){return false}if(this.isSkippedByAbsoluteNegativePatterns(e,r)){return false}return this.isMatchToPatterns(e.path,t)&&!this.isMatchToPatterns(e.path,r)};EntryFilter.prototype.isDuplicateEntry=function(e){return this.index.has(e.path)};EntryFilter.prototype.createIndexRecord=function(e){this.index.set(e.path,undefined)};EntryFilter.prototype.onlyFileFilter=function(e){return this.options.onlyFiles&&!e.isFile()};EntryFilter.prototype.onlyDirectoryFilter=function(e){return this.options.onlyDirectories&&!e.isDirectory()};EntryFilter.prototype.isSkippedByAbsoluteNegativePatterns=function(e,t){if(!this.options.absolute){return false}var r=n.makeAbsolute(this.options.cwd,e.path);return this.isMatchToPatterns(r,t)};EntryFilter.prototype.isMatchToPatterns=function(e,t){return i.matchAny(e,t)||i.matchAny(e+"/",t)};return EntryFilter}();t.default=o},47:function(e,t,r){"use strict";var n=this&&this.__importDefault||function(e){return e&&e.__esModule?e:{default:e}};Object.defineProperty(t,"__esModule",{value:true});t.MigrateEngine=t.EngineError=void 0;const i=n(r(2815));const o=r(3129);const s=n(r(5737));const a=n(r(422));const u=s.default("MigrateEngine:rpc");const l=s.default("MigrateEngine:stderr");const f=s.default("MigrateEngine:error Command failed with exit code 7. |
Good that it’s working! I don’t think I changed the logic in |
Prisma beta.8 is now in master. I successfully ran fresh install tests locally. (Made sure my App was using beta.8.) @jtoar I'll let you pull in changes from master to your branch |
@thedavidprice Update: I spun up Ubuntu v20 and v16 machines via digitalocean and successfully ran But my local machine's still being finicky. Sometimes the Prisma client seems to generate before the Just to be extra clear, everything still works in the end--there's just a moment where the client doesn't exist, api complains, and the dev server restarts. And I'm getting this error on a default 0.8.2 install; it doesn't seem to be something this branch introduced. The emulating local npm workflow also has no effect on this locally. I'll continue debugging tomorrow! |
Ah, in that case maybe what you're seeing is merely the difference in the time it takes for Prisma Client to generate. The DB, API, WEB processes are all being run in parallel, but API will throw if it's looking for Prisma Client and it doesn't exist (yet). dev-server could definitely use some TLC, but if you think this describes your case then I think it's ok for now. |
This sounds like what's happening. Seems like a linux or possibly an older-laptop problem (is that a thing?), because it works fine on a newer mac I tried. All the other checks have passed, testing and tutorial-wise. Anything left to do you think I'm missing in this PR @thedavidprice? |
Adds positional arguments and descriptions to arguments/options. This means that builder is now usually a function instead of an obejct. Other notes: - desc/describe -> description - adds terminal-link to makde terminal links - almost all commands get an .epilogue, linking to the CLI Reference - down/up have a new pos. arg: decrement/increment - modify dbCommands test for builder now being functions
@jtoar this is another epic PR. Thank you for the time + effort here. Reminder about future next steps:
|
This is a follow-up on #322: after making the CLI Reference, I audited the repo for consistency. I made some general changes and some specific ones. Based on what we do here I'll open a PR for the CLI Reference.
Positionals: I added positionals (explained in this blog post http://yargs.js.org/2017/10/20/yargs-10.html) across the board. This means that the exported
builder
const is now usually a function instead of an object.Many of the previous positional arguments were listed in help as options instead of arguments; take
build
as example:Before:
rw build [app..] Build for production. Options: --help Show help [boolean] --version Show version number [boolean] - --app [choices: "api", "web"] [default: ["api","web"]] --verbose, -v [boolean] [default: false] --stats [boolean] [default: false] Done in 0.90s.
After:
[app..] -> [side..]: I've changed the argument from
[app..]
to[side..]
inbuild
,dev
, andtest
because I thought that was the more correct term. Thoughts?desc, describe, description: Which one should we use? I've seen both
desc
anddescription
. Should we stick todesc
?Punctuating desc/describe/description? (Possibly trivial point no. 1): Should we end
desc
with periods or not? Again I've seen both.Linking to the CLI Reference: Yargs has an
.epilogue
method that adds a string to the end of the help message. I've been using this to link to the reference:I added terminal-link to do this.
Order of props in yargs.positional/option: (Possibly trivial point no. 2) The order of props in the
builder
const always seems to be slightly different. Should we keep it alphabetical? Doesn't matter?generate
The positional arg's currently
<type>
. For db, another entry-point command, it's<command>
.Type makes sense, but it'd be nice if the help said said Types instead of Commands:
I need to double check some things so I'll list this as a draft for now, but let me know what you think about the above.