-
Notifications
You must be signed in to change notification settings - Fork 181
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
How can we refactor our code? #137
Comments
I don't understand what zap-cli is. Can you add some use case? |
$ zap
Usage: zap <YouTube URL | video ID | search query> Background: you're a developer, and live in the terminal.
It might be interesting to see if we can play the video within the terminal somehow also |
I don't know about Node.js but couldn't it be done in a few lines in a shell script? Something like... get the status code of "curl -I input or http://youtubeblahblah...input" Why do we need to have so much dup code? edit :) |
Well, that's making an extra HTTP request. Doing it in Node gives us full cross-platform support, and it's actually not that much duplicated code but we can do better. |
If we do this, we'll need to break out the common logic - then include it via npm/bower. Submodules will be adding too much friction to the process
|
Is there downside of the extra HTTP request? It can let us fail early too. |
Not necessarily, but it's bad design and could add an unwanted amount of delay if the request takes over say 100ms. This isn't an option for the Chrome extension as it needs to be an instant operation, or it will be unusable. |
See #137 When this is actually done, js/zap-common.js will be moved into a bower module and end up moving to something like: bower_components/zap-common/index.js
See #137 When this is actually done, js/zap-common.js will be moved into a bower module and end up moving to something like: bower_components/zap-common/index.js
See #137 When this is actually done, js/zap-common.js will be moved into a bower module and end up moving to something like: bower_components/zap-common/index.js
Just merged #141! Now I just have to figure out how to move the contents of |
@shakeelmohamed are you looking to move zap-common into a Bower module that you can then import? |
@caseyprovost originally that was the idea. It seems that the JavaScript ecosystem has shifted from bower towards webpack and just using node modules - I think we can follow that model to stay modern. Out of that process, we might go ahead and port our codebase to es6 while we're at it if there's a compile step involved in the release process |
I made a terrible sin working on zap-cli by copy/pasting some code over to
contrib.js
.This is definitely not ideal, and entirely unmaintainable.
What can we do to avoid this?
I'm thinking we could break out utility functions into a new repository, then either
Use git submodules - will we need to use requireJS by doing so? (maybe not if our JS imports are in the right order)The text was updated successfully, but these errors were encountered: