Skip to content

feat: Add success/failure hooks to watch#130

Merged
jaredpalmer merged 16 commits into
jaredpalmer:masterfrom
hedgerh:feature/watch-hooks
Dec 19, 2019
Merged

feat: Add success/failure hooks to watch#130
jaredpalmer merged 16 commits into
jaredpalmer:masterfrom
hedgerh:feature/watch-hooks

Conversation

@hedgerh

@hedgerh hedgerh commented Jun 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@hedgerh hedgerh mentioned this pull request Jun 1, 2019
Closed
@jaredpalmer

jaredpalmer commented Sep 9, 2019

Copy link
Copy Markdown
Owner

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

@jaredpalmer

jaredpalmer commented Oct 11, 2019

Copy link
Copy Markdown
Owner

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

@swyxio

swyxio commented Dec 4, 2019

Copy link
Copy Markdown
Collaborator

i think i have rebased

@jaredpalmer

jaredpalmer commented Dec 19, 2019

Copy link
Copy Markdown
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 agilgur5 mentioned this pull request Dec 28, 2019
Merged
@agilgur5

agilgur5 commented Dec 28, 2019

Copy link
Copy Markdown
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 agilgur5 mentioned this pull request Dec 28, 2019
Merged
@agilgur5

agilgur5 commented Dec 31, 2019

Copy link
Copy Markdown
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 agilgur5 mentioned this pull request Jan 4, 2020
Closed
agilgur5 added a commit to agilgur5/tsdx that referenced this pull request Jan 4, 2020
@agilgur5
(fix): revert jaredpalmer#130's breaking change of tsconfig options
3325ee9 
- 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
@agilgur5 agilgur5 mentioned this pull request Jan 4, 2020
Merged
jaredpalmer pushed a commit that referenced this pull request Jan 5, 2020
@agilgur5 @jaredpalmer
(fix): revert #130's breaking change of tsconfig options (#415)
00f80ac 
- 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

swyxio commented Jan 24, 2020

Copy link
Copy Markdown
Collaborator

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

@allcontributors allcontributors Bot mentioned this pull request Jan 24, 2020
Merged
@allcontributors

allcontributors Bot commented Jan 24, 2020

Copy link
Copy Markdown
Contributor

@sw-yx

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

@agilgur5 agilgur5 mentioned this pull request Jan 31, 2020
Merged
@agilgur5 agilgur5 mentioned this pull request Feb 27, 2020
Closed
@agilgur5 agilgur5 mentioned this pull request Mar 19, 2020
Closed
@agilgur5

agilgur5 commented Apr 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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 agilgur5 mentioned this pull request Apr 8, 2020
Open
agilgur5 added a commit to agilgur5/tsdx that referenced this pull request Apr 11, 2020
@agilgur5 @kotarella1110
(clean): remove redundant set of watch opts
ed24fd4 
- 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 <kotarella1110@gmail.com>
@agilgur5 agilgur5 mentioned this pull request Apr 11, 2020
Merged
agilgur5 added a commit that referenced this pull request Apr 11, 2020
@agilgur5 @kotarella1110
(clean): remove redundant set of watch opts (#671)
351936e 
- 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 <kotarella1110@gmail.com>
@agilgur5 agilgur5 mentioned this pull request Oct 14, 2020
Merged
paul-vd pushed a commit to EezyQuote/tsdx that referenced this pull request Dec 1, 2020
@agilgur5 @kotarella1110 @paul-vd
(clean): remove redundant set of watch opts (jaredpalmer#671)
666635d 
- 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 <kotarella1110@gmail.com>
@agilgur5 agilgur5 mentioned this pull request Jan 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaredpalmer jaredpalmer jaredpalmer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Callbacks on Successful/Unsuccessful Build

4 participants

@hedgerh @jaredpalmer @swyxio @agilgur5