Skip to content
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

Migrate to NPM and Typescript #15

Merged
merged 21 commits into from
Jul 27, 2021
Merged

Conversation

rodydavis
Copy link
Member

  • Remove Bower and use NPM
  • Update README
  • Migrate JS code to TS
  • Add legacy dependencies in third_party
  • Rename app/js/ to app/ts/
  • Adding types where available
  • Added package.json and tsconfig.json

@rodydavis
Copy link
Member Author

@jverkoey

@appsforartists
Copy link
Member

appsforartists commented Jul 8, 2021

Really surprised to see action on this repo. Thanks for the effort!

A few notes from a quick look I took: (This diff is so big that GitHub's review UI is choking)

  • rodydavis/direct should be material-motion/direct in package.json

  • I see npm packages for angular-ui-sortable and jquery-ui. Can we use those instead, and totally remove bower + third_party?

  • The canonical script name to start a server is start. Can we please use that instead?

  • It's a real bummer that starting the server asks for your sudo password with no context. I haven't used AppEngine since the dev_appserver days, but I know they use gcloud now. I wonder if there's a command that doesn't require sudo, and doesn't require another massive refactor.

    Makes me wonder if we should port this to Firebase {Hosting, Firestore} (in a separate PR).

  • I believe Google Style is to use single quotes instead of double. (It's a total nit, but that's why I now use singles.) I wonder if https://github.com/google/gts would be helpful for this project.

  • It would also be nice if the server watched the filesystem and rebuilt when changes were made, but that may be out-of-scope for this PR.

- fixing name in package.json
@rodydavis
Copy link
Member Author

rodydavis commented Jul 14, 2021

  • rodydavis/direct should be material-motion/direct in package.json

Updated!

The ones on npm were not compiling and were not supporting the other packages. Basically it couldn't resolve the dependencies. This will be remove when I remove jquery and update angular.

  • The canonical script name to start a server is start. Can we please use that instead?

Updated!

  • It's a real bummer that starting the server asks for your sudo password with no context. I haven't used AppEngine since the dev_appserver days, but I know they use gcloud now. I wonder if there's a command that doesn't require sudo, and doesn't require another massive refactor.

I was trying to get the sdk to use the gcloud but not working at the moment. Maybe if we switch it to cloud run.

Makes me wonder if we should port this to Firebase {Hosting, Firestore} (in a separate PR).

I think that would be good. I am working to migrate it to the latest angular but right now it requires app engine.

  • I believe Google Style is to use single quotes instead of double. (It's a total nit, but that's why I now use singles.) I wonder if https://github.com/google/gts would be helpful for this project.

Installed the style guide but the tsconfig it extends is not outputting the correct ts, so I made the change to single quotes.

  • It would also be nice if the server watched the filesystem and rebuilt when changes were made, but that may be out-of-scope for this PR.

I added a watch command for ts and it is working. On terminal you run npm run ts:watch and the other npm run start.

@rodydavis
Copy link
Member Author

@appsforartists can you take another look?

@rodydavis
Copy link
Member Author

@jverkoey also if you could review

@appsforartists
Copy link
Member

Looks like most of the changes I recommended are in there. It's a bummer that you can't run ts:watch and sudo dev_appserver in the same command.

I think start should tell you why you're being asked for your password:

    "start": "npm run ts; echo \"sudo starting Google AppEngine\"; sudo dev_appserver.py .",

Also, the README needs to be updated with your changes; namely, start instead of serve and a note about running ts:watch and start in separate terminal sessions.

I don't have enough familiarity with Angular or Direct to give a more thorough review, but otherwise, LGTM. Thanks again!

@rodydavis
Copy link
Member Author

Updated messaging!

@appsforartists
Copy link
Member

Does bower install not need to be run anymore? I see the script is missing, but it's still listed in devDependencies

@rodydavis
Copy link
Member Author

Thats correct!

@rodydavis
Copy link
Member Author

@jverkoey

Copy link

@jverkoey jverkoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna apply a trust and verify lens here; once this merges let's make sure it deploys internally and then adjust anything in follow-ups as needed :)

@jverkoey jverkoey merged commit 6c1318c into material-motion:master Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants