add typescript config support #3835
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3835 +/- ##
=======================================
Coverage 98.12% 98.12%
=======================================
Files 201 201
Lines 7084 7087 +3
Branches 2072 2073 +1
=======================================
+ Hits 6951 6954 +3
Misses 64 64
Partials 69 69
Continue to review full report at Codecov.
|
|
Novel approach to be sure, kudos. I'm not sure what the side effects of rollup core depending on a plugin would be. Something to consider. Edit: This is a terrible idea, but to get the ball rolling on how we might make this easy for users without having core dependencies on plugins, we might consider something like a |
There was a problem hiding this comment.
That is definitely an interesting approach! Positive points:
- the impact to the Rollup code base is very low
- TS support will profit from improvements to the TypeScript plugin
However, the current implementation has some downsides:
- Mixing
requireinto an ES module can have some unintended effects. In any case, it "works" more by accident than anything else because Rollup will not touch therequireand is compiling for Node anyway. Should we ever have an ESM CLI build, this will break. - With this change, Rollup CLI will become unusable for anyone who does not have the typescript plugin installed. Which is especially bad for people who do not use the plugin.
- You used the deprecated
rollup-plugin-typescriptinstead of@rollup/plugin-typescript. I know that for Rollup, this is happening mostly for speed reasons (though I know there are much better alternatives these days). But otherwise any reason for not going with the proper plugin? This would also give us type-checking. - Always using the TypeScript plugin may have a slight negative speed impact for now TypeScript users
- There is no test.
- It should be mentioned in the documentation
So here are my suggestions:
- First there should be a test. In this case, this is as simple as adding a folder to
test/cli/samples. Take e.g.test/cli/samples/configas an example, and look at the other tests in this folder. - Load the TypeScript plugin via dynamic
import('@rollup/plugin-typescript')instead. As this is in an async function, you can easily await the import. - Mark the plugin as "external" by adding it to the
externalarray in/rollup.config.js. That way, Rollup will not try to bundle it. - Here is an important bit: Only
import()and add the plugin if the config file has a.tsextension. That way, you do not have the dependency or the parsing overhead when people are not using a.tsconfig file. Maybe that check can be done in theloadConfigFile()function where all other extensions are checked as well and be passed as a parameter togetDefaultFromTranspiledConfigFile(fileName, commandOptions.silent, extension === '.ts')? - Document that you can use TypeScript but that you need to have this plugin installed for it to work in
docs/01-command-line-reference.md. - Make sure that errors are handled in sufficiently nice way:
- TypeScript errors should be just displayed to the use
- When they are using a
.tsextension in their config file but the typescript plugin is not installed, the error should be sufficiently clear that they need to manually install that plugin if they want to use TypeScript in their config file.
- Optional but nice: Add the
tsextension to the default extensions checked infindConfigFileNameInCwdso that you can userollup -cfor TypeScript as well.
Would you be willing to add some or all of these changes?
|
@lukastaegert yes. |
|
@lukastaegert im not sure how to test the ts config because it needs the i will comment leave comments on the code to explain why i added it. |
cli/run/import-ts-plugin.ts
Outdated
| @@ -0,0 +1,11 @@ | |||
| import { resolve } from 'path'; | |||
|
|
|||
| export default async (configPath: string) => { | |||
There was a problem hiding this comment.
just using import() doesn't work, so i added this file which will be transformed into a normal node require import by the new build plugin i wrote.
There was a problem hiding this comment.
I would be interested to know why it would not work. Did you mark the plugin as external in the Rollup config? Rollup should automatically translate a dynamic import into a require wrapped in a promise if done right. The default export of the library should then be the default property of the resolved object
There was a problem hiding this comment.
i did add the plugin to the external array, the reason used babel is because in the compiled code it still used a dynamic import, i will try again maybe it will work this time.
There was a problem hiding this comment.
See my other comment, it is because you did not pass a static string to the import.
package.json
Outdated
| @@ -59,6 +59,7 @@ | |||
| "fsevents": "~2.1.2" | |||
| }, | |||
| "devDependencies": { | |||
| "@babel/core": "^7.12.3", | |||
There was a problem hiding this comment.
i couldn't get the dynamic import to work without adding a new plugin that uses babel, let me know if you think that there is a better way.
There was a problem hiding this comment.
See my other comment. I would have time to take a deeper look tomorrow
There is no problem with adding this plugin as a devDependency. Admittedly if this is a plugin is present, it will be harder to test the error message. But if you are feeling adventurous, the CLI tests should accept some sort of before and after hooks where you could rename the plugin before the test for the error and rename it back after the test. I could also have a look tomorrow if you want. |
im not sure what you mean, so i think its better if you make the test. |
|
@lukastaegert from an architectural point of view is it wise to be tightly coupling the config, and as a result rollup, to plugins? It strikes me that the second we support TS in core, there will be calls for other parser support, and alternatives to the official plugin (like those who believe that rollup-plugin-typescript2 is superior) I like the idea but I feel like this approach is being rushed, and I'd like to recommend taking a step back and viewing the problem from a higher level. the question we want to answer is: how can we empower users to leverage the rollup ecosystem in their rollup config, without making rollup core beholden to specific plugins or parsers. |
|
it works without babel, the problem was that for some reason the dynamic import doesn't get transpiled when its inline or in a function that is in the same file, but it does if its in another file. @shellscape i agree its better to be plugin agnostic, i think once we are all happy with the implementation i will create another branch so that we have a cleaner git history. |
|
If rollup doesnt bundle the config an easier path would be to use the nodejs loader ecosystem for things like this and others. |
|
@LarsDenBakker in the past I managed a similar system with webpack-cli (later webpack-command) and with webpack-dev-server, webpack-serve, and to some extent, webpack-nano and webpack-plugin-serve. All of those used the node loaders and packages like |
|
@shellscape yes, I see your point. So to go about this, we could allow the injection of general "config parsing plugins"? So how could it work:
We could actually still keep this low impact on the code base by making Fun fact: I just noticed that the code in But the advantages of going this way would be:
And wasn't there someone who wanted coffee-script support? Who would have thought we can finally go there now. Even FlowType support... What do you think @shellscape, @TheRealSyler? And @TheRealSyler thanks a lot for sticking with us so far, this would be a larger change for this PR. I still hope we can keep you on board with this. |
cli/run/import-ts-plugin.ts
Outdated
| export default async (configPath: string) => { | ||
| try { | ||
| return ( | ||
| await import(resolve(configPath, '../', 'node_modules/@rollup/plugin-typescript')) |
There was a problem hiding this comment.
Ah, the problem with the dynamic import is that you are constructing a URL here dynamically. This will indeed not work. If you just write import('@rollup/plugin-typescript' here it will work, though, as Node will take care of resolving it correctly at runtime.
And this will also pick the plugin from the right directory as Rollup injects the compiled config file into the Node module loader mechanism where the original config would have been.
But with the latest comments, maybe this will become obsolete anyway.
|
@LarsDenBakker to some extent, we are using the Node loader system (or rather the long-deprecated but never removed |
|
I understand why the configs are bundled by rollup historically, but this is no longer necessary since node 12+ supports native es modules. You can I think it would be better to align with standard modules instead of investing further in a custom system for rollup. The custom system has DX downsides too. Rollup es modules are different than node es modules (you can use Don't mean to hijack the discussions here, but just sharing my opinon here :) |
|
in we could also have default plugins for different file types so that for example if with this approach we don't have one function that has two jobs. |
|
my latest commit shows what how we could do it, the only problem with it is it works in theory but not in practice (with coffescript), i will fix it or figure out why it doesn't work. if you think that this is a good implementation i will make it pretty by adding proper error messages, tests etc. |
cli/run/getConfigPath.ts
Outdated
| @@ -33,7 +33,7 @@ export function getConfigPath(commandConfig: string | true): string { | |||
|
|
|||
| function findConfigFileNameInCwd(): string { | |||
| const filesInWorkingDir = new Set(readdirSync(process.cwd())); | |||
| for (const extension of ['mjs', 'cjs']) { | |||
| for (const extension of ['mjs', 'cjs', 'ts', 'coffee']) { | |||
There was a problem hiding this comment.
While TypeScript is fairly wide-spread, it feels weird to me to add special handling to CoffeeScript here.
cli/run/importConfigPlugin.ts
Outdated
| extension: string | ||
| ) { | ||
| let plugin = customPlugin; | ||
| if (!plugin || plugin === true) { |
There was a problem hiding this comment.
I think it is slightly awkward to support --customPlugin without any parameters and make it have the effect of adding the TypeScript plugin.
So this is indeed roughly what I originally envisioned (except for the added parameter), but did you give some thought of my suggestion of reusing the existing plugin logic? It would provide consistent syntax with the regular --plugin option and even allow ad-hoc plugins form the command line if someone desires.
Here is how it would work:
// in loadConfigFile.ts
//...
async function loadConfigFile(
fileName: string,
commandOptions: any
): Promise<GenericConfigObject[]> {
const extension = path.extname(fileName);
const configFileExport =
!commandOptions.configPlugun && extension === '.mjs' && supportsNativeESM()
? (await import(pathToFileURL(fileName).href)).default
: !commandOptions.configPlugun && extension === '.cjs'
? getDefaultFromCjs(require(fileName))
: await getDefaultFromTranspiledConfigFile(fileName, commandOptions);
return getConfigList(configFileExport, commandOptions);
}
//...
async function getDefaultFromTranspiledConfigFile(
fileName: string,
commandOptions: any
): Promise<unknown> {
const warnings = batchWarnings();
const inputOptions = {
external: (id: string) =>
(id[0] !== '.' && !path.isAbsolute(id)) || id.slice(-5, id.length) === '.json',
input: fileName,
onwarn: warnings.add,
plugins: [],
treeshake: false
};
addCommandPluginsToInputOptions(inputOptions, commandOptions, 'configPlugin');
const bundle = await rollup.rollup(inputOptions);
if (!commandOptions.silent && warnings.count > 0) {
//...
// In commandPlugins.ts
export function addCommandPluginsToInputOptions(inputOptions: InputOptions, command: any, pluginOption = 'plugin') {
if (command.stdin !== false) {
inputOptions.plugins!.push(stdinPlugin(command.stdin));
}
if (command.waitForBundleInput === true) {
inputOptions.plugins!.push(waitForInputPlugin());
}
const commandPlugin = command[pluginOption];
if (commandPlugin) {
//...
This would even support shortening the command to
rollup -c rollup.config.ts --configPlugin typescriptas it automatically tries the common plugin prefixes first.
Note that plugins are resolved relative to Rollup, not relative to the config file or current working directory as I first thought, but this would be no different for your implementation. Nice thing is that if we want to improve this later, we would improve it for both the --plugin and the --configPlugin option together.
|
i think this is a good implementation, with one downside which is if you always need to add |
|
with the latest commit you just need |
|
There were some concerns from @shellscape with regard to if this will collide with future plans, but he will take a few days to have a look. I hope you do not mind waiting a few days before we move on here because I really would love some feedback from him here. |
|
no problem i can wait. |
|
Is there any update on this thread? cc @shellscape |
|
It has been 8 months and it has not been confirmed yet. . . In my opinion, this is a lesson learned, just "copy homework".
|
|
I will try to have a look over the current state of this in the next days. My current feeling is this:
|
Well, it seems that I need to encapsulate higher-level cli tools based on rollup (just like vite does) ╮(╯-╰)╭ |
|
That is always a valid option. In think I will also have a look at |
Mainly, loading the ts configuration file is really simple, probably like this export function loadTypeScriptConfig(configPath: string) {
//Avoid using ts-node to run the error that leads to multiple compilation of ts, refer to: https://github.com/TypeStrong/ts-node/issues/883
if (!require.extensions['.ts']) {
// noinspection TypeScriptValidateJSTypes
require('ts-node').register()
}
return require(configPath)
}unit test import * as path from 'path'
import { loadTypeScriptConfig } from '../loadTypeScriptConfig'
it('testloadTypeScriptConfig', () => {
const start = Date.now()
const config = loadTypeScriptConfig(
path.resolve(__dirname, './test.config.ts'),
)
const end = Date.now()
console.log('config: ', JSON.stringify(config), ', ms: ', end - start)
})I also created a simple unit test example, you can take a look (so I don't understand why it is not supported...)
|
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install TheRealSyler/rollup#masteror load it into the REPL: |
|
I pushed the following changes:
This is good now from my side. I would consider releasing it in the next days as, beyond the question if "another way of using TypeScript for configs is better", I think this is a very useful feature to have. As the
Thus, the config file can share as many or as few plugins with the actual Rollup build as people want so that people can have a homogeneous code base. |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
i like writing my webpack config files in typescript (yes, you can do that), today i tried using rollup for a project because the new webpack dev server is even slower than before, after looking into the rollup source code i found a way to make it work not sure if this is the best way but it passes all tests (minus 1 that also fails without this change).
if there are any problems with my approach let me know, i don't expect that this pr will be merged as it is right now, its way to simple to actually work.
Update
In its final form, this PR now implements a new
--configPluginoption that works very similar to the--pluginCLI option and allows the user to specify plugins to be applied to the config file. This can be the TypeScript plugin, or any other Rollup plugin they like, e.g. CoffeeScript, Babel, Flow...