Skip to content
This repository has been archived by the owner on Jan 31, 2023. It is now read-only.

fix: Only enable TypeScript jsx compiling for .js, .jsx, and .tsx files #45

Merged
merged 4 commits into from
May 8, 2020
Merged

fix: Only enable TypeScript jsx compiling for .js, .jsx, and .tsx files #45

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a04432
Ignore JSX syntax in ts files.
sainthkh May 5, 2020
4a5153e
Add comment.
sainthkh May 7, 2020
ff71754
Flip the logic.
sainthkh May 8, 2020
ff55668
Make argument name more clear.
sainthkh May 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/simple_tsify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
let through = require('through2')
const through = require('through2')
const path = require('path')

const isJson = (code) => {
try {
Expand All @@ -16,6 +17,7 @@ const isJson = (code) => {
// We skip this slow type-checking process by using transpileModule() api.
module.exports = function (b, opts) {
const chunks = []
const ext = path.extname(b)

return through(
(buf, enc, next) => {
Expand All @@ -32,7 +34,7 @@ module.exports = function (b, opts) {
this.push(ts.transpileModule(text, {
compilerOptions: {
esModuleInterop: true,
jsx: 'react',
jsx: ext === '.ts' ? undefined : 'react',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes the default react and only turns it off if the extension is .ts. I think it should be flipped, so it's only set to react if the extension is .tsx (maybe .jsx too? not sure if that makes sense or is necessary). Seems more future-proof, so if some framework foo creates a TypeScript-compatible extension .tsf, it doesn't switch to react mode.

Copy link

Choose a reason for hiding this comment

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

'react' isn't the only option, but probably the most common. Should there be a way to configure this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrisbreiding

Then, how about changing it to ext === '.js' || ext === '.jsx' || ext === '.tsx'? Many developers use JSX syntax with .js files. I think it's safer to include .js files to the list.

@NicholasBoll

There are 4 options: undefined, "react", "preserve", "react-native".

But "preserve" and "react-native" don't make sense here. Cypress transpiles the test code and sends it to the browser and runs it. When we select "preserve" or "react-native" option, the code cannot be executed on browsers. Because JSX is not compiled and they're still <X /> in the transpiled code.

Currently, developers use .js, .jsx, .tsx extensions to include JSX syntax. And .ts doesn't allow JSX syntax. AFAIK, there is no other scenario. So, I don't think providing option to allow other jsx TypeScript compile options is an over-spec.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then, how about changing it to ext === '.js' || ext === '.jsx' || ext === '.tsx'

That sounds reasonable.

downlevelIteration: true,
},
}).outputText)
Expand Down
11 changes: 11 additions & 0 deletions test/fixtures/typescript/math_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ const { add } = math

const x: number = 3

const isKeyOf = <T>(obj: T, key: any): key is keyof T => {
sainthkh marked this conversation as resolved.
Show resolved Hide resolved
return typeof key === 'string' && key in obj;
}

context('math.ts', function () {
it('imports function', () => {
expect(add, 'add').to.be.a('function')
Expand All @@ -20,4 +24,11 @@ context('math.ts', function () {

expect(arr[0] + arr[1]).to.eq(1)
})
it('Test generic', () => {
const x = {
key: 'value'
}

expect(isKeyOf(x, 'key')).to.eq(true)
})
})