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

[WIP] Add custom transformer plugins #54278

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented May 16, 2023

For #14419
Fixes #54276

I have not finished fleshing out this PR; there's probably a lot wrong with how I am loading the new plugins, but I'd like to get feedback on the usability of this early while we iterate on the design and implementation.

I'll update the proposal and PR description with more info closer to completion.

Also, sorry, but I'm probably going to break my own rules and force push this branch; it futzes with the output files and adds a new d.ts and it'll be clearer with single commits. gerrit when?

@jakebailey
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 16, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 41fc675. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 16, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/154373/artifacts?artifactName=tgz&fileId=E84F858A7DB193E4718B04B36E180E9732E1BF8E0B89865EB1560119CDC550DA02&fileName=/typescript-5.2.0-insiders.20230516.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.2.0-pr-54278-2".;

Comment on lines +2711 to +2713
if (!host.require) {
return emptyArray;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe allow CompilerOptions to feed transformer factories directly, it will be better for API users.

@samchon

This comment was marked as off-topic.

@jakebailey
Copy link
Member Author

I'm not sure I understand your point; isn't the type checker available on Program?

@samchon
Copy link

samchon commented May 17, 2023

Oops, sorry for mis-reading.

I'd imported different type.

@jakebailey
Copy link
Member Author

However, current ts.CustomTransformerFactory does not provide the ts.Program instance.

It definitely does, it's passed to the create function.

Furthermore, as Node and SourceFile instance from typescript/lib/tsclibrary had removed most features, I can't do anything in typia.

Correct. The core compiler has a limited API.

@samchon
Copy link

samchon commented May 17, 2023

However, current ts.CustomTransformerFactory does not provide the ts.Program instance.

It definitely does, it's passed to the create function.

Furthermore, as Node and SourceFile instance from typescript/lib/tsclibrary had removed most features, I can't do anything in typia.

Correct. The core compiler has a limited API.

May I ask you the reason why limiting the API? I can't do anything with limited API.

@jakebailey
Copy link
Member Author

The API provided in this file is the API currently provided by tsc. The other files contain other APIs, including special allocators that add the extra methods to Node and Symbol, but slow down the compiler.

It's possible we will be removing those allocators, but there are other benefits to this distinction. Maybe we'll find ways around that too.

@samchon
Copy link

samchon commented May 17, 2023

I'm currently making a new branch in typia and testing it.

By the way, I don't know how much speedup there is by using typescript/lib/tsclibrary instead of typescript, but I'm very sad to find that there is no way for my typia and nestia. Even checking number type became impossible, due to ts.Type.getFlags() method has been erased.

I'm sorry for criticizing even from the 1st PR. However, type info is very important feature for my typia and nestia projects. Considering the purpose of the TypeScript project, I think this is nonsense. It just sounds me like "I removed the seatbelt and fuel efficiency went up."

if (config.type !== "transformer") return undefined;

// TODO(jakebailey): The LS plugin loader is more complicated than this; copy.
const result = host.require!(program.getCurrentDirectory(), config.path);
Copy link

@samchon samchon May 17, 2023

Choose a reason for hiding this comment

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

I think it should start from location of tsconfig.json file, but you're using current-working-directory.

This is the one of different part with ttypescript (and ts-patch).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is theoretically the tsconfig dir, but yes, this code is very WIP.

Copy link

Choose a reason for hiding this comment

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

Tested in test/tsconfig.json file and program.getCurrentDirectory() was typia/.

  • typia/
    • node_modules/
    • package.json
    • tsconfig.json
    • test/
      • tsconfig.json

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Oops, but all of this is going to be replaced anyway. This branch is effectively demo code at the moment.

@sosoba
Copy link

sosoba commented Jun 30, 2023

Hello. How about diagnostic messages? How plugin can send warnings and errors as DiagnosticMessage?

@sosoba
Copy link

sosoba commented Jun 30, 2023

And how about watch mode? Is it covered by another PR?

@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

@Anutrix
Copy link

Anutrix commented Jan 13, 2024

Any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A minimal custom transformer plugin proposal
6 participants