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

Throw when loading a TypeScript file that is not covered by the rewrite paths #26

Open
vedantroy opened this issue Apr 26, 2020 · 2 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@vedantroy
Copy link

vedantroy commented Apr 26, 2020

This is more like an anti-bug than an actual bug. But it seems like this project works even when I provide a nonsensical rewritePaths entry, for example:

"typescript": {
  "rewritePaths": {
    "meaninglessdirectory/": "jababababa/"
  }
},

Shouldn't ava crash if this occurs?

Here's the repository ("ava" branch): https://github.com/vedantroy/typecheck.macro

Check out the repository, switch to the ava branch, and then run npm i && npm run test.

What happens The tests run successfully. You can show that ava is not using the js file by renaming temp_build/test.js to temp_build/foobar.js and then executing npm run test:run. The tests will still execute. You can even modify tests/test.ts, maybe add an extra console.log statement, then run npm run test:run, and the new output will be printed. Does ava now support typescript natively, or am I missing something?

What you expected to happen: Ava should crash or something because inside of my package.json, I have a meaningless typescript configuration.

Ava version: 3.7.1

@novemberborn
Copy link
Member

Oh this is interesting!

It works… because tests/test.ts is valid JavaScript. As soon as you put some TypeScript syntax in there it crashes.

This line stops the TypeProvider from being the one to load the test file:

return testFileExtension.test(ref) && rewritePaths.some(([from]) => ref.startsWith(from));

I think maybe canLoad() should return true as long as the extension matches, but then in load() if we can't find a rewrite path we should throw an error:

const [from, to] = rewritePaths.find(([from]) => ref.startsWith(from));

And then we can state that all selected TypeScript test files must be covered by the rewrite paths.

What do you think @vedantroy?

@novemberborn novemberborn added bug Something isn't working help wanted Extra attention is needed labels Apr 27, 2020
@vedantroy
Copy link
Author

vedantroy commented Apr 27, 2020

I think that's a good idea! Doing extra checks is preferable to letting things work by accident ☺️

I would try submitting a PR for this, but unfortunately I'm a bit busy at the moment.

@novemberborn novemberborn changed the title This project works even when I provide a gibberish rewritePaths entry? Throw when loading a TypeScript file that is not covered by the rewrite paths Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants