-
Notifications
You must be signed in to change notification settings - Fork 435
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
Refactor clasp push #514
Refactor clasp push #514
Conversation
Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
Cool! Can we figure out how to use absolute import paths rather than relative (tsconfig?)? } from '/commands/auth'
// Rather than
} from './../auth' |
I looked into using absolute paths, but it doesn't look feasible at this time. We could use absolute path mappings:
as suggested by TS documentation (see https://www.typescriptlang.org/docs/handbook/module-resolution.html). However, take a look here at the
This actually now tries to resolve the modules in the There's a bunch of discussion here about the issue microsoft/TypeScript#10866 We could use a 3rd party library, but I'd prefer not to. |
@erickoledadevrel have you had a chance to give this a look? Thanks! |
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.
Sorry for the delay!
src/commands/push.ts
Outdated
@@ -0,0 +1,105 @@ | |||
import { | |||
loadAPICredentials, |
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.
Nitpick: Should this be collapsed into one line, since there is only one import happening?
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 could be. I figured to keep it multi-line to stay with the style of the other imports. I can change to one-line.
src/commands/push.ts
Outdated
@@ -0,0 +1,105 @@ | |||
import { |
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.
Given all the imports added to this new file, are there now any unused imports in commands.ts
? I can't tell just by looking at this PR, but feels like there is a good chance that at least a few of these imports aren't needed by the remaining code.
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.
There are many unused imports in commands.ts
(unrelated).
The only one that was moved over from commands.ts
to push.ts
that is now unused in commands.ts
is PROJECT_MANIFEST_FILENAME
from utils
. I can remove it.
* TODO: Only push the specific files that changed (rather than all files). | ||
* @param cmd.watch {boolean} If true, runs `clasp push` when any local file changes. Exit with ^C. | ||
*/ | ||
export default async (cmd: { watch: boolean, force: boolean }) => { |
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.
Was there any changes made to the actual code, or was it just copied and pasted as-is? Hard to tell with the diff view.
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.
Yes, great question.
The constants manifestHasChanges
and confirmManifestUpdate
were extracted to the bottom of the file and comments were added to them.
My thought is that it improves code readability. Additionally, if we wanted in the future, we could export these and test them individually.
Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
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.
Looks good!
Pulled
clasp push
out and refactored its internal functions a bit.Signed-off-by: campionfellin campionfellin@gmail.com
Works on #501
npm run test
succeeds.npm run lint
succeeds.