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

Add compile option #12

Merged
merged 22 commits into from
Apr 12, 2021
Merged

Add compile option #12

merged 22 commits into from
Apr 12, 2021

Conversation

szmarczak
Copy link
Contributor

@szmarczak szmarczak commented Feb 3, 2020

Fixes #3

Precompile TS files before running tests. Uses execa, because the TS Compiler API is too complicated.

index.js Outdated Show resolved Hide resolved
@szmarczak szmarczak marked this pull request as ready for review February 3, 2020 21:06
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@szmarczak
Copy link
Contributor Author

What directory does this write to? Is this based on the configuration? It might be good to override that — if you have AVA watch mode running, and your own tsc -w it may lead to trouble if both are updating the same output directory.

I checked that. Running double tsc -w gives no errors. Perhaps it waits until the file is unlocked and then compiles the project again?

@szmarczak szmarczak changed the title Add precompile option Add compile option Feb 4, 2020
@Conaclos
Copy link
Contributor

Conaclos commented Feb 4, 2020

Interesting work!
Why did you choose execa instead of the native exec? Is there any advantage here?

@szmarczak
Copy link
Contributor Author

Why did you choose execa instead of the native exec? Is there any advantage here?

Plenty of reasons, just check out its repo here to see what it has to offer: https://github.com/sindresorhus/execa

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

I checked that. Running double tsc -w gives no errors. Perhaps it waits until the file is unlocked and then compiles the project again?

I don't know. But we can ship this and find out.

So the idea here is that AVA does the compilation, but you still need to configure the rewrite paths. Fair enough, we have #2 for that.

You need to git-ignore the build info file that TypeScript creates, and git rm the file.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/tsconfig.json Outdated Show resolved Hide resolved
@novemberborn
Copy link
Member

@szmarczak I've pushed some tweaks.

I'm concerned about compilation being noticeably slow. Perhaps we need to force users to make a decision as to whether to have this provider compile? That is, not allow the compile configuration to be undefined?

We also need a way of communicating errors. Right now we log the ExecaError and it's pretty horrible. We could check if tsc exited with code 1 and then share the stdout with AVA, but we probably need to make some changes in AVA itself so it can neatly print that stdout.

And finally we need to update the documentation here.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

(Forgot to request changes along with posting the above comment.)

@szmarczak
Copy link
Contributor Author

I've pushed some tweaks.

Great! :D I agree on all the points, I'll try to finish the PR this week (I'm quite busy with preparing for the matura exam I'm having in <3 months).

@novemberborn novemberborn mentioned this pull request Feb 10, 2020
@novemberborn
Copy link
Member

@szmarczak now that we're invoking tsc, we shouldn't have to pass it the tsconfig.json file right?

@szmarczak
Copy link
Contributor Author

now that we're invoking tsc, we shouldn't have to pass it the tsconfig.json file right?

I think so. I'll do this now :)

index.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Thanks for coming back to this @szmarczak!

This is a breaking change, right?

What is the error output when you have not configured compile?

I assume the tsc output is printed to the console, during the test run? Is that something we want? Should we only print error output? Does it interact with any spinners AVA prints?

README.md Outdated Show resolved Hide resolved
@szmarczak
Copy link
Contributor Author

This is a breaking change, right?

Yes.

I assume the tsc output is printed to the console, during the test run?

No. execa doesn't output to stdout by default.

Should we only print error output?

If the build fails then the compileTypescript promise will throw. I'm not sure whether we should proxy stderr to stdout or not. Or we can leave it as it is. Then the user will need to run tsc manually.

@szmarczak
Copy link
Contributor Author

@novemberborn
Copy link
Member

If the build fails then the compileTypescript promise will throw. I'm not sure whether we should proxy stderr to stdout or not. Or we can leave it as it is. Then the user will need to run tsc manually.

I'd rather not interleave the output. What is the experience when there is an error? Is it graceful or confusing?

What is the error output when you have not configured compile?

#12 (files)

What do you think about checking if config.compile === undefined and asking users to set that property? Especially after an upgrade this will be a likely error.


I'd be curious to hear your thoughts on the discussion in #29. Especially around changing this to compile: 'tsc' | false.

@szmarczak
Copy link
Contributor Author

This is the message the provider throws:

## compile() error

> Snapshot 1

    `Command failed with exit code 2: tsc --incremental␊
    typescript/typescript.ts(1,1): error TS2304: Cannot find name 'a'.`

I'm assuming AVA already passes the errors from providers to stderr. Let me know if I have to do something else.

What do you think about checking if config.compile === undefined and asking users to set that property? Especially after an upgrade this will be a likely error.

Sounds good.

@szmarczak
Copy link
Contributor Author

What do you think about checking if config.compile === undefined and asking users to set that property? Especially after an upgrade this will be a likely error.

Done :) The errors are friendly now.

@szmarczak
Copy link
Contributor Author

Is isPlainObject really needed? Can't we just check typeof value === 'object' && value !== null?

@novemberborn
Copy link
Member

Is isPlainObject really needed? Can't we just check typeof value === 'object' && value !== null?

I like it, it forces the config to be as described, without allowing arbitrary values through.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Thanks @szmarczak. I've made the unrelated breaking changes in a separate PR, fixed some bugs, and added the 'tsc' compilation option. I've also tweaked the readme a bit. What do you think?

@szmarczak
Copy link
Contributor Author

Awesome 🎉

@novemberborn novemberborn merged commit 869760a into avajs:main Apr 12, 2021
@fregante
Copy link

Neat! I'll be testing this soon. Thanks!

Does anyone know if this already works with type: module?

@novemberborn
Copy link
Member

Does anyone know if this already works with type: module?

#5 is still open.

@szmarczak
Copy link
Contributor Author

Just updated Got to use @ava/typescript 2.0.0 - works great!

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.

Perform compilation
4 participants