fix: Stop fresh builds from failing

[jarjee]

I had to fix the build before I could even get started having a look at implementing a PR for #61.

This is based in part off the work done in #69, although I've taken a different approach to handling the errors. In Typescript you can solve the issue with Errors being unknown with instanceof narrowing. This has the benefit of matching what we're already doing in index.ts, and being typesafe to boot.

I did have to make use of some dodgy casting in the print.ts tests, as far as I can tell console-testing-library has a fix, but the latest released package doesn't have the change.

I've also just gone ahead and made a package-lock.json for this project, while I know locks were purged in #17, the fact that locks were removed meant that unimported suffered bitrot due to packages updating and making breaking changes in their minor versions (I'm looking at you typescript).

It might be worth also looking at updating the github workflow (the cy.yml) to use npm ci instead of npm i when downloading dependencies since it should be faster & encourage the use of the lockfile, but I'm not going to make changes there unless requested.

[smeijer]

Hi! Thanks for the PR. Unfortunately, the tests are failing in the Github workflow. Can you take a look at those?

It might be worth also looking at updating the GitHub workflow (the cy.yml) to use npm ci instead of npm i ... but I'm not going to make changes there unless requested.

Sounds good to me. Thanks.

[jarjee]

Ok, I've made the change to use npm ci instead of npm i.

Another change I made for the tests is to move away from v12 node. The reason for this is it's going to reach EOL in about a month from now (2022-04-30). Making the tests run on v14 was easier than trying to change how the tests use fs/promise.

I'm not sure if there's anything else that'll fail. I can pass the tests fine with node v14 on my machine, but using act to simulate the github actions, I have them failing on a flow test. Not entirely sure why that's happening to be honest.

I'm running act with act pull_request --job test to run only the test job, and had to mangle the job definition a little since act seems to not like caches.

[jarjee]

Ah, just realised that using npm ci might interact badly with the cache plugin, since it's going to wipe node_modules every time it runs.

I'm not familiar with the cache github action, so not sure if that then affects all subsequent cache action jobs.

Turns out it'll be fine, we only run npm ci on a cache miss

[jarjee]

I tweaked the cache to use package-lock.json, since that should now be the source of truth for the state of the repo.

What isn't so good is that it's looking like the test for should support flow type is flaky, since it passed at least once. I'll have a look later at why it's inconsistent, or I'll use the sledgehammer that is jest.retryTimes()

[jarjee]

Just wanted to give an update; I have been working on this on and off but I did get pretty busy recently.

Yes, the error is real, no sadly jest.retryTimes() doesn't fix it.

The issue seems to be specifically when stripping out type and typeof imports, which is currently handled by the included patch.

I do have a working but unmergable solution: use a horrifying regex to fix the imports before the flow types get removed:

    code = code.replace(
      /import[^;\n]*type[^;\n]*from[^'"]*(["'][^'";]*["'])(;*)/g,
      'import X from $1$2',
    );
    
    code = handleFlowType(code, config);

More realistic solutions:

• I possibly update the remove-flow-types library and make another custom patch just for imports, although it's likely this is what caused the original regression
• Bite the bullet and just write an import mangler using the flow parser.

As an aside, we should probably be using // @flow and /* @flow */ to look for flow files, not the file extensions.

[smeijer]

Sorry I've been in the dark for a while. Thanks so much for all you're doing!

we should probably be using // @flow and /* @flow */ to look for flow files, not the file extensions.

I'm not sure if that's enough, as I believe those annotations aren't required if a project uses flow for all the files.

[smeijer]

The changes to package-lock.json and the GitHub actions, somehow made the CI fail. I suspect that it's just a cache issue, so I'll apply those changes later, separately. I'm going ahead and merge this.

Thanks so much!