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

Support adding additional compiler options #311

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

EliLichtblau
Copy link

This small PR adds the ability for users to supply additional tsconfig options to merge with the tsconfig that is used.

This is useful in the context of the monorepo where I can use vite resolve alias to get hmr passed package boundary however if I change the type signature I do not get type hints.

See this example repo - when I make a change in formatters I want a declaration emit compile so I can get the types and vite do its normal hot loading.

I would really appreciate it if you consider merging - thanks so much!

Copy link

changeset-bot bot commented Apr 4, 2024

🦋 Changeset detected

Latest commit: d2f5cb5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
vite-plugin-checker Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Apr 4, 2024

Deploy Preview for vite-plugin-checker failed.

Name Link
🔨 Latest commit d2f5cb5
🔍 Latest deploy log https://app.netlify.com/sites/vite-plugin-checker/deploys/6620645554c9a7000815658e

@jhoermann
Copy link
Contributor

Seems like f620186 fixes the Pipeline - so #264 is fixed then @fi3ework. As this repo uses conventional commits consider rewriting commit messages @EliLichtblau.


if (typeof pluginConfig.typescript === 'object' && pluginConfig.typescript.buildMode) {
const host = ts.createSolutionBuilderWithWatchHost(
ts.sys,
createProgram,
/* eslint-disable */
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Why disable eslint @EliLichtblau?

Copy link
Author

Choose a reason for hiding this comment

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

Theres a lint rule that makes it impossible to make a function that takes I think more than 3 arguments iirc - this needs to take 6

Copy link
Contributor

Choose a reason for hiding this comment

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

Try using Rest parameters instead. Disabling eslint should be the last resort i guess.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe philosophical difference but I disable eslint when making eslint happy makes the code harder to understand. Imo rest parameters make this code that is currently clear less clear. If the maintainer asks me I'll happily make the change but until then I think disabling eslint is probably the right call

/**
* Additional Compiler options to merge with tsconfig
*/
extraTSOptions: ts.CompilerOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Are docs changed needed here @EliLichtblau?

Copy link
Author

Choose a reason for hiding this comment

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

I can change the docs if wanted - I thought the comment would be clear in intellisense - I think the mistake here is I didn't make "extraTSOptions" optional which was an oversight

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be added to the docs here I think.

Copy link
Author

Choose a reason for hiding this comment

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

thanks! will do now

Copy link
Contributor

Choose a reason for hiding this comment

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

Added in ee392c7

@EliLichtblau
Copy link
Author

@jhoermann Do you not squash/rebase. on merge? I can change the title to be "conventional commit" or I can change all of them if needed. Let me know!

@jhoermann
Copy link
Contributor

@jhoermann Do you not squash/rebase. on merge? I can change the title to be "conventional commit" or I can change all of them if needed. Let me know!

Squashing should work - depends on what @fi3ework thinks as I'm not the owner of this repo.

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.

2 participants