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

TypeScript support #130

Closed
KubaJastrz opened this issue Apr 6, 2020 · 7 comments · Fixed by #131
Closed

TypeScript support #130

KubaJastrz opened this issue Apr 6, 2020 · 7 comments · Fixed by #131
Labels

Comments

@KubaJastrz
Copy link
Contributor

Hi, I wanted to start working on testing-library/dom-testing-library#494 but it seems like kcd-scripts doesn't support transpilation of TypeScript files.

  • kcd-scripts version: 5.6.1
  • node version: 12.11.0
  • npm (or yarn) version: N/A

What you did:

I cloned https://github.com/testing-library/dom-testing-library, renamed src/index.js to src/index.ts and started working on enabling TS support.

Changing input file was easy:

cross-env BUILD_INPUT=src/index.ts kcd-scripts build

I've added typescript and @babel/preset-typescript dependencies and babel config:

// babel.config.js
module.exports = {
  presets: ['kcd-scripts/babel', '@babel/preset-typescript'],
}

This is where the issue comes. Because kcd-scripts doesn't support overriding rollup plugins, I can't add extensions: ['.js, '.ts'] to rollupBabel and nodeResolve plugins. Also, I need to use runtimeHelpers: true for some reason, but I use custom babel config so this line is stopping me as well:

runtimeHelpers: useBuiltinConfig,

Problem description:

I need to customize some plugins in rollup configuration.

Suggested solutions (pick one):

  1. Support TypeScript extensions and change runtimeHelpers: true under some condition. A flag maybe? Or automatic detection based on input file extension?
  2. Same as above but without any condition, I don't think it will add that much build overhead but this is something to check.
  3. Allow overriding certain rollup plugins.

This is how I visualize 3rd option:

// kcd-scripts' rollup.config.js
const builtinPlugins = {babel: rollupBabel(...), resolve: nodeResolve(...), ...};
module.exports = { 
  input, 
  output,
  ..., 
  plugins: Object.values(builtinPlugins),
  builtinPlugins,
}
// custom usage rollup.config.js
const rollupConfig = require('kcd-scripts/dist/config/rollup.config')
const rollupBabel = require('rollup-plugin-babel')

rollupConfig.builtinPlugins.babel = rollupBabel({
  extensions: ['.js', '.ts'],
  runtimeHelpers: true,
}),

rollupConfig.plugins = Object.values(builtinPlugins)
module.exports = rollupConfig
@kentcdodds
Copy link
Owner

Hi @KubaJastrz,

Thanks for your work! Maybe we should set the runtimeHelpers to whether they have a dependency on @babel/runtime. That seems more logical.

We have a utility for that.

Want to make a PR?

@KubaJastrz
Copy link
Contributor Author

Sure, I can take a look.

What about file extensions? Should we just include them always?

@kentcdodds
Copy link
Owner

Yeah, I think that's the simplest and doesn't really have a negative impact.

@KubaJastrz
Copy link
Contributor Author

KubaJastrz commented Apr 9, 2020

Okay, so the only step for the developer would be to create a custom babel.config.js with presets: ['kcd-scripts/babel', '@babel/preset-typescript'] but we can add that to the docs I think.

@kentcdodds
Copy link
Owner

Yeah, I think that's it. Though we might be able to add that preset automatically if we detect a tsconfig.json. I can't think of a downside to that, so why don't we do that too, so then it works seamlessly :)

@KubaJastrz
Copy link
Contributor Author

True, I've actually implemented that as well but basing on typescript dependency. Will submit a PR once I verify it works correctly.

@kentcdodds
Copy link
Owner

🎉 This issue has been resolved in version 5.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants