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

Fix typescript rechecking after files change. #4187

Merged
merged 7 commits into from
Feb 25, 2020
Merged

Fix typescript rechecking after files change. #4187

merged 7 commits into from
Feb 25, 2020

Conversation

astegmaier
Copy link
Contributor

@astegmaier astegmaier commented Feb 19, 2020

↪️ Pull Request

This PR contains a possible fix to #4185. The goal is to enable @parcel/validator-typescript to correctly re-check files when they change, instead of remaining stuck with the errors it found the first time it ran.

I'm putting it out there for early feedback. Unit tests are not done yet, but I'd love any pointers to tests that need to validate similar things (e.g. that something re-ran when a file changed), or general advice. Edit (2/24): I successfully added a unit test, and made changes to make sure it works on Windows (it initially did not).

One other small nit was: I wasn't super happy with the way that lint-staged+prettier formatted the lines I added. For example, it broke apart lines 70-72 in an unhelpful way. I want to conform to whatever code styling guides that are defined project-wide, but I was wondering if there is some easy way to tweak this (if not, not worries - the code works). Edit (2/24): Unfortunately it doesn't look like there's an easy way to tweak this, so I moved on.

💻 Examples

First, you need to configure @parcel/validator-typescript to run with the following .parcelrc`

{
    "extends": "@parcel/config-default",
    "validators": {
        "*.{ts,tsx}": ["@parcel/validator-typescript"]
    }
}

Then, if you have a file that contains a type error, for example:

index.ts

const message: number = "Hello World!" // This is a type error!

...and you run parcel index.ts, you'll see the error correctly reported. However when you fix the error and save the file, the validator will re-run, but the old error will still be reported. With this change, however, you'll see the error go away in the terminal.

🚨 Test instructions

See the old errors branch of this repo for a repo where the original problem can be seen. If you clone this branch, build it, and yarn link @parcel/validator-typescript to that test repository, you can see the problem go away.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for looking into this.

Feel free to add another unit test, if you don't have the time we can merge as is.

For the test you can probably have a look at how hmr tests are written

@astegmaier
Copy link
Contributor Author

astegmaier commented Feb 22, 2020

@padmaia and/or @DeMoorJasper - I think this is ready to merge as far as functionality and unit tests go. I know @DeMoorJasper already approved it, but since it's my first merge, and some things have changed since the initial approval, I wanted to get at least one of you to give me the go-head out of an abundance of caution.

I'm a bit puzzled by the test suite failures in Azure Pipelines - I don't get them on my Windows or Mac machines locally, and I saw that the last merge (#4193) also had similar failures. Is this a known issue? If yes, is it okay to merge with the outstanding errors?

EDIT (2/20): I tried to update my forked repo from the main repo by running git rebase upstream/v2, and I may have screwed something up - now previous things that were committed previously to the main repo are now showing up as part of this PR. I'll try to sort this out (although if I can't, I might end up closing this PR and creating a new, clean one). EDIT (2/24): This is now fixed.

Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

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

Just gonna block this due to the merge issue you had, don't wanna mess up v2.

As far as I can see it looks good, will do another review once that merge issue is resolved

@astegmaier
Copy link
Contributor Author

I fixed the merge issues (sorry for that!), so I think it's good-to-go.

(The problem was that I ran rebase more than once and picked up changes from the main v2 repo each time, which then got re-played on top of already-committed stuff. Lesson learned!)

@DeMoorJasper DeMoorJasper merged commit 7418b91 into parcel-bundler:v2 Feb 25, 2020
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