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

Scripts Review/Clean up #25

Closed
7 of 8 tasks
kwelch-eb opened this issue Apr 10, 2018 · 7 comments · Fixed by #26
Closed
7 of 8 tasks

Scripts Review/Clean up #25

kwelch-eb opened this issue Apr 10, 2018 · 7 comments · Fixed by #26
Assignees

Comments

@kwelch-eb
Copy link
Contributor

kwelch-eb commented Apr 10, 2018

I have been thinking hard on this and would love to know your opinion.

Short Version/Action List

  • Remove test:watch
  • Remove tsc
  • Remove test:ci
  • Add --ci tag to travis script
  • Remove check:size from validate
  • Add check:size as postbuild
  • Update validate:watch to include tsc runner
  • [Optional] Remove validate:watch to start
  • [ ] [Optional] Replace tsc with jest based option

Long Version

I feel as though our scripts should be based on publish consumption and use case. With that in mind I recommend we remove tasks that are not for publish use, specifically the CI test script.

I believe our scripts should be based on the phase of work. Here is a summary of the phased and my recommendation on how we should handle them, including specific action items.

  • Development Phase
    • This should be a minimal script that ensure continued code functionality and surface errors and warnings
      • For this reason, I recommend we remove check:size from validate because it does not add a lot of value while running in a watch state
    • Covered by either validate (one time) or start/validate:watch (watch mode)
      • I recommend we remove test:watch as it is covered our watch script
  • Build Phase
    • Covered by build
    • This is a good final check of the code and is best suited for longer actions and full compliation
    • I recommend move the check:size to the build phase as it already requires a build and its data is most pertinent then.
  • Deploy Phase
    • This is currently intended to be resolved and completed by CI and no public scripts are necessary

Note: This is still a lot of raw thoughts and I look forward to discussion and flushing this out.

@benmvp
Copy link
Contributor

benmvp commented Apr 10, 2018

I think all of your suggestions make sense. I really like having start for the watch mode. Makes a lot of sense for library development.

The reason we had tsc was so that we can use npm-run-all in check:static. It would be nice to get rid of.

If we get rid of test:watch & test:ci, we would still keep test though right?

I'm thinking maybe calling check:size in build instead of with postbuild.

Totally +1 with "Deploy Phase"

Thanks!

@kwelch-eb
Copy link
Contributor Author

The standard scripts are start and test so I would picture test staying even if we remove the "sub"-scripts.

I am indifferent to the location of check:size in build vs postbuild. The main point on that it I see it more as a verification than a validation step. I think having it in build/postbuild would mean we don't need a precheck:size to run a build.

If we are good with this, I will start making some of these changes. I will probably make a few PRs for this since that list is a lot of small tasks.

@kwelch-eb kwelch-eb self-assigned this Apr 10, 2018
@rwholey-eb
Copy link

+1 to removing check:size from anything thats run by a watcher. I'm all for less scripts. I'm not sure i see the benefits of a few smaller prs though since the dev team so far is pretty small. Thoughts on changing the scripts all at once?

@benmvp
Copy link
Contributor

benmvp commented Apr 10, 2018

I assumed this was happening in a single PR...

@kwelch-eb
Copy link
Contributor Author

I am good with doing them all at the same time.

@kwelch-eb
Copy link
Contributor Author

I have started on this a bit. Slightly blocked by azz/jest-runner-tsc#2, but going to move forward with most of it then double back to see if I can get the config passing through properly.

kwelch-eb pushed a commit that referenced this issue May 9, 2018
simplify scripts and move to jest-runner-tsc

* update travis to pass ci flag
* clean up scripts section
* remove static check script
* add ci to build script
* add tsc jest runner
* updated jest-runner-tsc to latest
* remove ci from build
* added type-check script

fixes #25
@ebtravis
Copy link
Collaborator

🎉 This issue has been resolved in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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 a pull request may close this issue.

4 participants