Skip to content

Load js config file #51613

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

Closed
wants to merge 4 commits into from
Closed

Load js config file #51613

wants to merge 4 commits into from

Conversation

ANFADEV
Copy link

@ANFADEV ANFADEV commented Nov 21, 2022

Fixes #30400 , #39441 and potentially #37884

I was coding a helper for Prebuilder like this one for Rollup,
It needs a javascript config file, in order to let Prebuilder control the compilation process with dynamic paths.
I was surprised to see that it's still not possible in typescript, so i dug in this repo this weekend, and came up with this solution, that apparently satisfies some of the points expressed in #30400:

  • it's fairly secure, the js config is obtained by converting the tsconfig.js' default import to a json string, and then treated the same as a tsconfig.json
    • So no function will be used by the compiler, just raw json-like data
    • Any config error handling already in place for a .json config works for the .js one too (i tested, putting an inexistant compiler option)
  • it's not complicated, as it only adds one step to the process
  • in order to use a js config file, tsc will need it to be specified using the -p parameter, so the default behavior is preserved

Testing

I tried writing a test, but it has not been easy, i spent more time trying to do this, than the actual implementation...
I would leave this task to any willing person, as i couldn't figure where to put the .js config file

I have tested manually like this:

  • built with npm run build
  • tested with npm run test (✔)
  • prepared with npx hereby LKG
  • published the package to my local registry
  • created a test project with two .ts scripts and the tsconfig.js (below) and without a tsconfig.json file in the project
  • ensured that the test project ran tsc using the local typescript package i published

And it worked perfectly, transpiling to the 'dist' folder.✔
Config extensibility (extends: "./tsconfig.js",) works with js config files too ✔

Usage

tsc -p tsconfig.js

tsconfig.js :

module.exports = {
    compilerOptions: {
      declaration: true,
      target: "es5",
      module: "commonjs",
      strict: true,
      noUnusedLocals: true,
      noUnusedParameters: true,
      noImplicitReturns: true,
      noFallthroughCasesInSwitch: true,
      composite: true,
  
      outDir: "dist/prod",
    },
    include: [ "./src/" ]
}

Also:
Dynamically adding a path ✔
And extending with a .js config ✔

SET NODE_ENV=dev&& tsc -p tsconfig-extended.js
const path = require('path');

module.exports = {
    extends: "./tsconfig.js",
    compilerOptions: {
        // transpiles to the 'dist/dev' folder
        outDir: path.join("dist", process.env.NODE_ENV),
    }
}

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 21, 2022
@MartinJohns
Copy link
Contributor

#30400 (comment)

We are not accepting PRs to turn config into an executable thing.

@ANFADEV
Copy link
Author

ANFADEV commented Nov 21, 2022

Plenty of people want this feature, it's one of the reasons the issue was reopened
see comment:

I am reopening since this has so much interest. I closed it initially out of respect for the maintainer’s statements that they didn’t want to support it.

It's incredibly useful, please reconsider...

As for the "can of worms" problem, there isn't any really.. typescript would only treat the import as a json object, if it fails at converting it to json it falls back, and reports an error to the user.

A lot of other dev tools like Webpack, Rollup, Vite etc.. even support config plugins, which would be more "hazardous" than this implementation.
So i don't see an imperative reason to worry about this

@MartinJohns
Copy link
Contributor

MartinJohns commented Nov 21, 2022

It's incredibly useful, please reconsider...

It's up to the team, I just quoted them.

As for the "can of worms" problem, there isn't any really.. typescript would only treat the import as a json object, if it fails at converting it to json it falls back, and reports an error to the user.

It executes the JavaScript. So what happens if there are imports in the js file? Or if someone accesses process? If this feature is supported, people will expect these things to work as well (at least the import), and the team would have to support it.

And the config is also used for tooling support, e.g. when you open a project in VS Code it reads your configuration. With this change it means you open your workspace and it executes JavaScript - that's a security no-go. What's the alternative? Only support .js in CLI? People will expect it to work, and not to have two configs. Try to blacklist/whitelist code in the file? That's never going to work safely.

@ANFADEV
Copy link
Author

ANFADEV commented Nov 21, 2022

So what happens if there are imports in the js file? Or if someone accesses process? If this feature is supported, people will expect these things to work as well

Imports in the js file will be executed as the developer wants, like in my latest example:

SET NODE_ENV=dev&& tsc -p tsconfig-extended.js
const path = require('path');
module.exports = {
    compilerOptions: {
        outDir: path.join("dist", process.env.NODE_ENV),
    }
}

which would be read by typescript as:

{
    "compilerOptions":{
         "outDir": "dist/dev",
     }
}

Imports and anything else, can't have a way into typescript's process, as the read output of the js file is a json string


when you open a project in VS Code it reads your configuration. With this change it means you open your workspace and it executes JavaScript

Well yes, VS Code prompts the user a modal to check if the user trusts the content of the project already,

Devs put only what's needed in config files, see how rollup & webpack configs have been used til now by the dev community.
A developer is responsible for his .js config file, just as he is for coding a script an putting it in the package.json

Denying this control to his configuration process, isn't gonna stop him from putting malicious code in his own project, if he wills to

@ANFADEV
Copy link
Author

ANFADEV commented Nov 21, 2022

This could also solve #37884

@typescript-bot
Copy link
Collaborator

The TypeScript team hasn't accepted the linked issue #30400. If you can get it accepted, this PR will have a better chance of being reviewed.

@sandersn
Copy link
Member

sandersn commented Dec 2, 2022

I would much prefer to design this feature in an issue instead of a PR. The code is pretty simple, but the design is complicated. Since the original issue is still open, let's discuss there. Although I personally still agree with @RyanCavanaugh 's hesitation to have an executable config.

In the meantime, to help with PR housekeeping, I'm going to close this PR.

@sandersn sandersn closed this Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Allow javascript (or typescript) config file
4 participants