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 typescript config support #3835

Merged
merged 17 commits into from Jun 16, 2021
Merged

add typescript config support #3835

merged 17 commits into from Jun 16, 2021

Conversation

@TheRealSyler
Copy link
Contributor

@TheRealSyler TheRealSyler commented Oct 23, 2020

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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 --configPlugin option that works very similar to the --plugin CLI 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...

@codecov
Copy link

@codecov codecov bot commented Oct 23, 2020

Codecov Report

Merging #3835 (ac35be2) into master (bfae791) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
src/utils/options/mergeOptions.ts 100.00% <ø> (ø)
cli/run/commandPlugins.ts 97.36% <100.00%> (ø)
cli/run/getConfigPath.ts 89.47% <100.00%> (ø)
cli/run/loadConfigFile.ts 96.22% <100.00%> (+0.22%) ⬆️

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 bfae791...ac35be2. Read the comment docs.

@shellscape
Copy link
Contributor

@shellscape shellscape commented Oct 23, 2020

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 .rolluprc file that would specify plugins to use with the config files.

Copy link
Member

@lukastaegert lukastaegert left a 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 require into an ES module can have some unintended effects. In any case, it "works" more by accident than anything else because Rollup will not touch the require and 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-typescript instead 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/config as 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 external array 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 .ts extension. That way, you do not have the dependency or the parsing overhead when people are not using a .ts config file. Maybe that check can be done in the loadConfigFile() function where all other extensions are checked as well and be passed as a parameter to getDefaultFromTranspiledConfigFile(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 .ts extension 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 ts extension to the default extensions checked in findConfigFileNameInCwd so that you can use rollup -c for TypeScript as well.

Would you be willing to add some or all of these changes?

@TheRealSyler
Copy link
Contributor Author

@TheRealSyler TheRealSyler commented Oct 24, 2020

@TheRealSyler
Copy link
Contributor Author

@TheRealSyler TheRealSyler commented Oct 24, 2020

@lukastaegert im not sure how to test the ts config because it needs the @rollup/plugin-typescript to be in the node_modules folder, which also the main downside of the current implementation, for now i only tested that i throws the correct error.

i will comment leave comments on the code to explain why i added it.

@@ -0,0 +1,11 @@
import { resolve } from 'path';

export default async (configPath: string) => {
Copy link
Contributor Author

@TheRealSyler TheRealSyler Oct 24, 2020

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.

Copy link
Member

@lukastaegert lukastaegert Oct 24, 2020

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

Copy link
Contributor Author

@TheRealSyler TheRealSyler Oct 24, 2020

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.

Copy link
Member

@lukastaegert lukastaegert Oct 25, 2020

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",
Copy link
Contributor Author

@TheRealSyler TheRealSyler Oct 24, 2020

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.

Copy link
Member

@lukastaegert lukastaegert Oct 24, 2020

See my other comment. I would have time to take a deeper look tomorrow

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Oct 24, 2020

im not sure how to test the ts config because it needs the @rollup/plugin-typescript to be in the node_modules folder,

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.

@TheRealSyler
Copy link
Contributor Author

@TheRealSyler TheRealSyler commented Oct 24, 2020

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.

@shellscape
Copy link
Contributor

@shellscape shellscape commented Oct 24, 2020

@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.

@TheRealSyler
Copy link
Contributor Author

@TheRealSyler TheRealSyler commented Oct 24, 2020

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.

@LarsDenBakker
Copy link
Contributor

@LarsDenBakker LarsDenBakker commented Oct 24, 2020

If rollup doesnt bundle the config an easier path would be to use the nodejs loader ecosystem for things like this and others.

@shellscape
Copy link
Contributor

@shellscape shellscape commented Oct 24, 2020

@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 rechoir. It worked, but it comes with a heavy dependency and DX cost. If we could let users use rollup plugins to enhance their configs, without making core dependent upon having a plugin dependency and without having core looking for a certain plugin, that would be the sweet spot for DX and environment control.

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Oct 25, 2020

@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:

  • It needs to be triggered from the command line as of course rollup cli needs to know about this before we go about reading the config file. E.g. --configPlugin @rollup/plugin-typescript. Thus, users could also e.g. use sucrase or even esbuild here if they have it wrapped in a rollup plugin
  • The effect would be to
    • Force that Rollup is bundling the config file
    • Dynamically load the plugins

We could actually still keep this low impact on the code base by making addCommandPluginsToInputOptions slightly more generic by adding a third parameter what the name of the option is that contains potential plugins (i.e. commandPlugin vs. plugin). That way, we would have the same syntax as the existing --plugin option. But instead of applying it to the final input options, we apply it to the input options of the config parsing.

Fun fact: I just noticed that the code in commandPlugins.ts is also using require to load plugins. Talking about consistent reviews, I totally overlooked this... So maybe something I have a look at at another time.

But the advantages of going this way would be:

  • This PR does not need to concern itself with loading plugins at all, as the code is already there
  • Testing becomes very easy as you can just use different plugin names.

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.

export default async (configPath: string) => {
try {
return (
await import(resolve(configPath, '../', 'node_modules/@rollup/plugin-typescript'))
Copy link
Member

@lukastaegert lukastaegert Oct 25, 2020

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.

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Oct 25, 2020

@LarsDenBakker to some extent, we are using the Node loader system (or rather the long-deprecated but never removed require.extensions that everyone and their Jest seems to be using). It is just we are applying Rollup as transpiler for ESM to CJS here, so adding additional plugins to that step kindof makes sense. And as Rollup is really fast for small builds, Rollup can really show its strengths here.

@LarsDenBakker
Copy link
Contributor

@LarsDenBakker LarsDenBakker commented Oct 25, 2020

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 require or import a config based on file extension or package type. (We use https://github.com/modernweb-dev/web/tree/master/packages/config-loader for this in our tools).

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 require and access __dirname for example), stack traces from errors thrown in the config don't show the correct file and line numbers and globals such as __dirname reference the bundled file location rather than the source file.

Don't mean to hijack the discussions here, but just sharing my opinon here :)

@TheRealSyler
Copy link
Contributor Author

@TheRealSyler TheRealSyler commented Oct 25, 2020

in loadConfigFile.ts/loadAndParseConfigFile() the addCommandPluginsToInputOptions function gets called after the config gets loaded, so my idea is we use the newly added import-ts-plugin.ts and make more generic so in getDefaultFromTranspiledConfigFile(fileName, commandOptions.configPlugin, commandOptions.silent, extension === '.ts' ); we change the check to extension !== '.js'then we parse the config with the plugin passed via the configPlugin option.

we could also have default plugins for different file types so that for example if extension === '.ts' the default is @rollup/plugin-typescript.

with this approach we don't have one function that has two jobs.

@TheRealSyler
Copy link
Contributor Author

@TheRealSyler TheRealSyler commented Oct 25, 2020

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.

@@ -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']) {
Copy link
Member

@lukastaegert lukastaegert Oct 28, 2020

While TypeScript is fairly wide-spread, it feels weird to me to add special handling to CoffeeScript here.

extension: string
) {
let plugin = customPlugin;
if (!plugin || plugin === true) {
Copy link
Member

@lukastaegert lukastaegert Oct 28, 2020

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 typescript

as 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.

@TheRealSyler
Copy link
Contributor Author

@TheRealSyler TheRealSyler commented Oct 29, 2020

i think this is a good implementation, with one downside which is if you always need to add --configPlugin typescript to the command, which is something that i don't really like, i will add a new commit with a default --configPlugin implementation, then we can see if that is something we want to go with.

@TheRealSyler
Copy link
Contributor Author

@TheRealSyler TheRealSyler commented Oct 29, 2020

with the latest commit you just need rollup -c, if you have a typescript config file and it can be extended for other config types, the small downside with this apporach is that we have lots of function arguments that get passed from one function to the next while only getting used by the last function in the chain.

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Oct 29, 2020

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.

@TheRealSyler
Copy link
Contributor Author

@TheRealSyler TheRealSyler commented Oct 29, 2020

no problem i can wait.

@JounQin
Copy link

@JounQin JounQin commented Apr 20, 2021

Is there any update on this thread? cc @shellscape

@rxliuli
Copy link
Contributor

@rxliuli rxliuli commented Jun 6, 2021

It has been 8 months and it has not been confirmed yet. . . In my opinion, this is a lesson learned, just "copy homework".

  • gulp: Use ts-node to load ts configuration files
  • vite: package vite.config.ts => vite.config.ts.js, then load the js file

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jun 6, 2021

I will try to have a look over the current state of this in the next days. My current feeling is this:

  • We should not (yet) have out-of-the-box support for TypeScript as we do not want hard dependencies on specific plugins or techniques, at least not yet.
  • Otherwise, I think the --configPlugin way should be the way forward for now. The effect would be that it forces the config to be transpiled with Rollup. In the future, we may add another technique of using loaders maybe using the builtin APIs and deprecate this one in favour of the other one.

@rxliuli
Copy link
Contributor

@rxliuli rxliuli commented Jun 6, 2021

I will try to have a look over the current state of this in the next days. My current feeling is this:

  • We should not (yet) have out-of-the-box support for TypeScript as we do not want hard dependencies on specific plugins or techniques, at least not yet.
  • Otherwise, I think the --configPlugin way should be the way forward for now. The effect would be that it forces the config to be transpiled with Rollup. In the future, we may add another technique of using loaders maybe using the builtin APIs and deprecate this one in favour of the other one.

Well, it seems that I need to encapsulate higher-level cli tools based on rollup (just like vite does) ╮(╯-╰)╭

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jun 6, 2021

That is always a valid option. In think I will also have a look at loadConfigFile, which is an API created just for secondary tooling.

@rxliuli
Copy link
Contributor

@rxliuli rxliuli commented Jun 6, 2021

That is always a valid option. In think I will also have a look at loadConfigFile, which is an API created just for secondary tooling.

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...)

https://github.com/rxliuli/web-project-tools/blob/6e2944b8ed/apps/liuli-cli/src/utils/loadTypeScriptConfig.ts#L6-L7

reference: https://inspirnathan.com/posts/30-ts-node-tutorial/#Using-ts-node/register

@github-actions
Copy link

@github-actions github-actions bot commented Jun 14, 2021

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install TheRealSyler/rollup#master

or load it into the REPL:
https://rollupjs.org/repl/?pr=3835

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jun 15, 2021

I pushed the following changes:

  • Remove builtin TypeScript support. This removes the implicit dependency to @rollup/plugin-typescript. Thus, all config languages are "equal" and users should just use the Rollup plugin they use anyway.
  • Make sure that when the --configPlugin option is used, the config is always transpiled.
  • Extend documentation to describe the new CLI option and link to it in appropriate places

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 --configPlugin option reuses the logic from the --plugin option, it supports all its features:

  • You can specify multiple plugins
  • You can pass options to plugins

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.

@lukastaegert lukastaegert merged commit 9a2775b into rollup:master Jun 16, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants