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

feat: Add success/failure hooks to watch #130

Merged
merged 16 commits into from
Dec 19, 2019

Conversation

hedgerh
Copy link
Contributor

@hedgerh hedgerh commented Jun 1, 2019

Resolves #104

Adds flags to the tsdx CLI for running commands on success, first success, and failure of builds when running watch. Usage:

tsdx watch --onFirstSuccess "echo First successful build" 
tsdx watch --onSuccess "echo Successful build"
tsdx watch --onFailure "echo The build failed" 

Still thinking about how to test these, and maybe even the watch command in general.

PS. I also added a start command in the tsdx project to run tsc in watch mode to re-bundle on changes.

@jaredpalmer
Copy link
Owner

I agree we should add this. Can you fix the conflicts?

@jaredpalmer
Copy link
Owner

Let's fix conflicts and merge. This would be super useful for a bunch of stuff.

@swyxio
Copy link
Collaborator

swyxio commented Dec 4, 2019

i think i have rebased

@jaredpalmer
Copy link
Owner

lgtm.

@jaredpalmer jaredpalmer changed the title tsdx watch: add success/failure hooks feat: Add success/failure hooks to watch Dec 19, 2019
@jaredpalmer jaredpalmer merged commit ab54278 into jaredpalmer:master Dec 19, 2019
@agilgur5
Copy link
Collaborator

So the esnext target override was changed in this PR -- is that safe?? It doesn't seem related to the other changes.

I'm not 100% sure why the esnext target override was set as such to begin with (it seems to exist since some of the first commits / original versions of TSDX), but I thought it existed so that TS would minimally make changes and then Babel (via preset-env) would further transpile to the target. I'm not really sure if that assumption is correct, but I think it did that before, so this is potentially breaking if correct.

If that's not correct, this still now makes the tsconfig target option actually applied, which is a non-trivial change as well and I think also potentially breaking (e.g. if the newly applied target doesn't transpile some features that were transpiled before), but I'm not 100% sure about how the interaction with preset-env works.

@agilgur5
Copy link
Collaborator

@jaredpalmer so this got released in 0.12.1 -- could you comment on the above, re: safety/potential breaking-ness of changing the target override?

agilgur5 added a commit to agilgur5/tsdx that referenced this pull request Jan 4, 2020
- not sure why this change was made in that PR as it was unrelated to
  the rest, but it broke some things related to Babel usage and maybe
  other things
  - e.g. babel macros were not included per jaredpalmer#413
  - and it used `tslib` instead of adding babel helper functions

- add a comment of why it seems like the target is overriden to esnext
  - though I am not sure of the original reasoning, as it has existed
    since very early versions of TSDX, see jaredpalmer#130 comments
jaredpalmer pushed a commit that referenced this pull request Jan 5, 2020
- not sure why this change was made in that PR as it was unrelated to
  the rest, but it broke some things related to Babel usage and maybe
  other things
  - e.g. babel macros were not included per #413
  - and it used `tslib` instead of adding babel helper functions

- add a comment of why it seems like the target is overriden to esnext
  - though I am not sure of the original reasoning, as it has existed
    since very early versions of TSDX, see #130 comments
@swyxio
Copy link
Collaborator

swyxio commented Jan 24, 2020

thanks for the great PR! @all-contributors please add @hedgerh for ideas, docs, code, tests

@allcontributors
Copy link
Contributor

@sw-yx

I've put up a pull request to add @hedgerh! 🎉

@agilgur5
Copy link
Collaborator

agilgur5 commented Apr 8, 2020

Argh, found another oversight in this PR while looking at #669. This ended up adding these lines to watch:

    opts.name = opts.name || appPackageJson.name;
    opts.input = await getInputs(opts.entry, appPackageJson.source);

Which are redundant with the normalizeOpts function that is called above it 😕

agilgur5 added a commit to agilgur5/tsdx that referenced this pull request Apr 11, 2020
- another thing that was missed in jaredpalmer#130
- these sets are already done in normalizeOpts, no need to redundantly
  perform them again
  - fortunately this wasn't inconsistent with normalizeOpts, so no
    harm done before this got out

Co-Authored-By: Kotaro Sugawara <[email protected]>
agilgur5 added a commit that referenced this pull request Apr 11, 2020
- another thing that was missed in #130
- these sets are already done in normalizeOpts, no need to redundantly
  perform them again
  - fortunately this wasn't inconsistent with normalizeOpts, so no
    harm done before this got out

Co-Authored-By: Kotaro Sugawara <[email protected]>
paul-vd pushed a commit to EezyQuote/tsdx that referenced this pull request Dec 1, 2020
- another thing that was missed in jaredpalmer#130
- these sets are already done in normalizeOpts, no need to redundantly
  perform them again
  - fortunately this wasn't inconsistent with normalizeOpts, so no
    harm done before this got out

Co-Authored-By: Kotaro Sugawara <[email protected]>
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.

Callbacks on Successful/Unsuccessful Build
4 participants