Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

chore(package): use @babel/register instead of ts-node #1395

Merged
merged 9 commits into from May 27, 2019

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented May 27, 2019

Motivation

We are using ts-node to compile our TS code on the fly during Gulp tasks and Screener run. As we we're switching to use Babel (#508), we can use @babel/register as it is faster than ts-node (there are no type checks during run).

This change is required for #1387, otherwise TS configs will become complicated and hard to maintain: we're calling Gulp with different configs that in package's root which results us to strange build commands:
"build": "cross-env TS_NODE_PROJECT=../../tsconfig.json gulp bundle:package:no-umd --package react-component-ref"

Remove custom TSLint rule

We don't need it more as these cases are covered by react/no-unknown-property 🚀

allowSyntheticDefaultImports in /build

This rule allows you to use ES style default imports even when the code you’re importing doesn’t have an ES default export. This happens when you import, for example, React, whose code is not written in ES (the source code is, but React ships a built version). This means that it technically doesn’t have an ES default export, so TypeScript will tell you so when you import it.

We can't enable this option in our main project (react) because we can produce broken typings (#46) for our consumers who don't enable this option. But it's safe to enable it for our tools, otherwise it becomes endless war with Babel because it handles CommonJS modules differently:

// Module
module.exports = () => {}

// Babel
import cx from 'module' // correct
import * as cx from 'module' // TypeError: express is not a function

// Typescript
import cx from 'module' // Typings error: no default export
import * as cx from 'module'

More in Babel REPL.

We had the same issue before even in distributed code (classnames #570, https://github.com/stardust-ui/react/blob/v0.31.0/packages/react/src/lib/felaExpandCssShorthandsPlugin.ts#L7).

create-react-app enables allowSyntheticDefaultImports: true and skipLibCheck: true by default: https://github.com/facebook/create-react-app/blob/v3.0.1/packages/react-scripts/scripts/utils/verifyTypeScriptSetup.js#L106

@codecov
Copy link

codecov bot commented May 27, 2019

Codecov Report

Merging #1395 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1395   +/-   ##
=======================================
  Coverage   73.47%   73.47%           
=======================================
  Files         778      778           
  Lines        5859     5859           
  Branches     1726     1726           
=======================================
  Hits         4305     4305           
  Misses       1548     1548           
  Partials        6        6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0ca943...3d5bf21. Read the comment docs.

@DustyTheBot
Copy link
Collaborator

Warnings
⚠️ There are no updates provided to CHANGELOG. Ensure there are no publicly visible changes introduced by this PR.

Generated by 🚫 dangerJS

@@ -1,35 +0,0 @@
import * as Lint from 'tslint'
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't need this more, is catched by react/no-unknown-property

16:28  error  Unknown property 'stop-color' found, use 'stopColor' instead  react/no-unknow
n-property

@layershifter layershifter changed the title WIP: Chore/babel register chore(package): use @babel/register instead of ts-node May 27, 2019
@layershifter layershifter merged commit 0730931 into master May 27, 2019
@delete-merged-branch delete-merged-branch bot deleted the chore/babel-register branch May 27, 2019 11:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants