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

feat: support postcss.config.ts #218

Merged
merged 7 commits into from
Jun 14, 2021
Merged

feat: support postcss.config.ts #218

merged 7 commits into from
Jun 14, 2021

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented May 28, 2021

Notable Changes

This lands support for postcss.config.ts 🎉

Commit Message Summary (CHANGELOG)

feat: support `postcss.config.ts`

Type

  • Feature

SemVer

  • Fix (:label: Patch)
  • Feature (:label: Minor)
  • Breaking Change (:label: Major)

Issues

Checklist

  • Lint and unit tests pass with my changes
  • I have added tests that prove my fix is effective/works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes are merged and published in downstream modules

@G-Rath
Copy link
Contributor Author

G-Rath commented May 28, 2021

The types should be mostly correct, but plugins & other dependencies require some upstream updates to be v8 compatiable i.e. DefinitelyTyped/DefinitelyTyped#53003

@ai
Copy link
Member

ai commented May 28, 2021

Thanks. Looks great.

But can I also ask you to add ESM export test for postcss.config.ts with "module": "esnext" in tsconfig.json?

@G-Rath
Copy link
Contributor Author

G-Rath commented May 28, 2021

@ai I can do an approximation of that, but it won't be completely possible without upgrading jest and configuring module support (i.e you need to run node with --experimental-vm-modules) since using esnext means that the test suite has to be able to use import/export.

I can implement the use of interopRequireDefault which should be the key thing required to provide nice support for both modules & commonjs.

@ai
Copy link
Member

ai commented May 28, 2021

Yeap, you can update Jest and change how we run tests

@G-Rath
Copy link
Contributor Author

G-Rath commented May 28, 2021

I don't think this is possible without making a breaking change, as you have to set "type": "module" in the root package.json, which means require cannot be used anywhere (not to mention iirc this impacts downstream users on the module).

I'm happy to explore this, but think it would be better to land support for this as is (as that matches how other libraries like jest, webpack, karma, ajv, etc are supporting it), and then look at refactoring the codebase for native ESM support as a follow up.

@ai
Copy link
Member

ai commented May 28, 2021

I asked this, because I'm afraid that ts-node is a bad solution.

A few months ago, there was no way to use ts-node with ESM in TS. If it is still a problem, can we switch to another method?

@G-Rath
Copy link
Contributor Author

G-Rath commented May 29, 2021

I'm afraid that ts-node is a bad solution.

Is that a statement or a concern? My experience with ts-node so far (which includes my using it to implement support for configs written in TypeScript for a number of tools) has been pleasant and I don't know of any other solution that's as light weight or universal (i.e you could do it with babel, but that's a huge dependency to have for this feature; you could also probably do it with something like esbuild, but then that means you're pulling in binaries that have OS requirements as well as not having type-checking).

My understanding is that ESM is now supported at a similar level to Node.

I can confirm I've been able to create a project with module set to esnext which ts-node is able to correctly execute.

@ai
Copy link
Member

ai commented May 29, 2021

Great news. Yeap, let’s do it this way (I will need a day to merge and release it).

@ai
Copy link
Member

ai commented May 31, 2021

I will play with ts-node and ESM support on my hobby project first. I hope to release it next week.

This allows both `module.exports =` & `export default` to be used
@G-Rath
Copy link
Contributor Author

G-Rath commented May 31, 2021

Awesome - let me know if there's anything I can help with :)

test/ts.test.js Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated

// Register TypeScript compiler instance
try {
registerer = require('ts-node').register()
Copy link
Member

Choose a reason for hiding this comment

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

Can we call require('ts-node') only if .ts file as found?

I do not want to load heavy TypeScript for the projects with .js configs, but with ts-node in the system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have a look, but this doesn't actually load the heavy side of the TypeScript compiler until it's actually needed, making it a very cheap call.

Copy link
Member

Choose a reason for hiding this comment

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

Am I right that we are calling require('ts-node').register() on any config loading?

(I may be wrong, I am not sure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; like I said the require cost is negligible since it doesn't actually do any work (including creating the TypeScript compiler, and compiling code, which are the expensive operations) until a .ts file is required.

However I think we might be able to move it into the loader function.

@ai
Copy link
Member

ai commented Jun 13, 2021

Sorry, for the delay. I am back to review the PR.

@G-Rath G-Rath requested a review from ai June 13, 2021 20:55
@ai
Copy link
Member

ai commented Jun 13, 2021

LGTM. Hope to merge and release it tomorrow (sorry, I am moving from one country to another, don’t have a lot of time for open source this month).

@ai ai merged commit e364e93 into postcss:master Jun 14, 2021
@ai
Copy link
Member

ai commented Jun 14, 2021

Thanks. Released in 3.1.

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

Successfully merging this pull request may close these issues.

Allow typescript config
2 participants