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(vite-node): options via CLI (fixes #1208) #1215

Merged

Conversation

antoinerey
Copy link
Contributor

@antoinerey antoinerey commented Apr 30, 2022

Fixes #1208

This PR adds a new flag to the vite-node CLI: --server-options. Using dot syntax, it's now possible to define any option passed to the underlying ViteNodeServer instance.

Because CLIs do not support RegExp as input, we make sure to transform any string that looks like /xxx/ to RegExps.

@netlify
Copy link

netlify bot commented Apr 30, 2022

Deploy Preview for vitest-dev ready!

Name Link
🔨 Latest commit 1ce975b
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/62932621c321c6000891a747
😎 Deploy Preview https://deploy-preview-1215--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@antoinerey antoinerey changed the title feat(vite-node): Add --inline-deps flag feat(vite-node): Add --inline-deps flag (fixes #1208) Apr 30, 2022
Comment on lines 81 to 83
deps: {
inline: Array.isArray(inlineDeps) ? inlineDeps : [inlineDeps],
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we also port the rest of the relevant configuration as CLI flags? Or would you rather keep the bare minimum, and add stuff when needed?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should at least port fallbackCJS (with no- prefix for false) - I think minimist provides this functionality out of the box, but I am not sure.

Also, what about regexp?

packages/vite-node/src/cli.ts Outdated Show resolved Hide resolved
Comment on lines 81 to 83
deps: {
inline: Array.isArray(inlineDeps) ? inlineDeps : [inlineDeps],
},
Copy link
Member

Choose a reason for hiding this comment

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

I think we should at least port fallbackCJS (with no- prefix for false) - I think minimist provides this functionality out of the box, but I am not sure.

Also, what about regexp?

@sheremet-va
Copy link
Member

sheremet-va commented May 1, 2022

I could not find any test associated to vite-node.

I don't think we explicitly tested vite-node 🤔 One of the reason is because vite-node was previously inlined in vitest, and I guess no one ported tests, when we split it into 2 packages.
Maybe we can add a vite-node folder to test.

I've only taken care of adding --inline-deps, but should we also port all/most of the relevant configuration that people may define under the test property in their Vitest config file?

Not all are relevant there, but maybe we can provide the ones that are not used in cli file right now (when defining new ViteNodeServer).

@antfu
Copy link
Member

antfu commented May 2, 2022

Maybe we could introduce a field like viteNode inside vite.config.ts for vite-node?

For CLI options, I am imagining something more automated, like --deps.inline=... and --x.y= mapping the structure of the configurations so we don't need to introduce new options every time ppl need them.

@antoinerey
Copy link
Contributor Author

For CLI options, I am imagining something more automated, like --deps.inline=... and --x.y= mapping the structure of the configurations so we don't need to introduce new options every time ppl need them.

Oh yes, good point. I'll try to look into it.

Maybe we could introduce a field like viteNode inside vite.config.ts for vite-node?

I'm still trying to get a feeling of how it could work from a consumer perspective, especially how we could avoid duplicating the configuration between vitest (the test part), and vite-node (the viteNode part). Is this duplication really an issue, or is it a tradeoff that you're willing to accept?


Side notes:

  • Let's not forget to add a similar type system as vitest through a triple slash command.
  • Let's investigate wow we could automatically translate TS types into CLI documentation.

@sheremet-va
Copy link
Member

@antoinerey I think cac (cli tool used in Vitest) already supports object-like parameters. We can just pass them directly to options, I think (maybe with some processing).

ViteNode right now uses minimist, so it will require a small rewrite.

@antoinerey
Copy link
Contributor Author

I'll definitely take a look at that! Thanks for the tip.

@antoinerey
Copy link
Contributor Author

Here is the first PR to replace minimist with cac. #1249

@antoinerey antoinerey force-pushed the feat/add-inline-deps-option-to-vite-node branch from 73770d7 to f1e8323 Compare May 10, 2022 07:14
@@ -24,6 +28,7 @@ export interface CliOptions {
root?: string
config?: string
watch?: boolean
serverOptions?: ViteNodeServerOptions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the option is provided by the consumer, it might not match ViteNodeServerOptions, so I'm cheating a little bit here.

However, we could rely on a TS guard to assert that the given options match the TS interface, but it would mean hardcoding the checks, and thus duplicating the source of truth between ViteNodeServerOptions and the TS guard. Concretely, both could be out of sync, leading to issues.

Do you see another way to achieve this without duplicating the source of truth? 🤔

Comment on lines 16 to 18
// TODO: How could we document this? Ideally, we should link to the
// TODO: ViteNodeServerOptions type since it's the source of truth.
.option('--server-options <options>', 'Use specified Vite server options')
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 could not find any proper example of such documentation. Should I add a few lines to the README?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think that would do

})
}

// TODO: Handle serverOptions.transformMode.
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've noticed that serverOptions.transformMode.ssr (and .web) only accept Regexes, while serverOptions.deps.inline (and .external) accepts both strings and Regexes.

  • Is that expected?
  • If so, would you mind giving a bit of context as why? 🙏

Copy link
Member

Choose a reason for hiding this comment

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

doesn't have a reason 👀
I guess just didn't notice. but to be honest I prefer using regexp always, because current solution for deps.inline as a string is not reliable on non-npm/pnpm package managers

@antoinerey antoinerey marked this pull request as draft May 10, 2022 07:22
@antoinerey
Copy link
Contributor Author

Converted the PR to draft since it's still WIP, and should probably not be merged.


@sheremet-va @antfu I've pushed a new version of this PR using cac as CLI, and it now handles arbitrary options through dot nesting. 🎉

vite-node --server-options.deps.inline="module-1" --server-options.deps.external="module-2" script.ts

Before going further (adding tests, polishing the code, etc.) I would like to get feedbacks, especially about ideas discussed in comments. Do you feel like this is going into the right direction? 🙏

chaii3 pushed a commit to chaii3/vitest that referenced this pull request May 13, 2022
@antoinerey
Copy link
Contributor Author

@sheremet-va Friendly ping. 🤗

Do you feel like a TS guard could be a nice solution even though it duplicates the shape of the options? Or should we keep a single source of truth instead?

@sheremet-va
Copy link
Member

Do you feel like a TS guard could be a nice solution even though it duplicates the shape of the options? Or should we keep a single source of truth instead?

I think the whole idea why we are doing this is to just do it and forget kind of situations.

@antoinerey antoinerey force-pushed the feat/add-inline-deps-option-to-vite-node branch from 3d9d2b3 to 2a96ef8 Compare May 17, 2022 07:59
@antoinerey antoinerey changed the title feat(vite-node): Add --inline-deps flag (fixes #1208) feat(vite-node): Add support for ViteNodeServerOptions (fixes #1208) May 18, 2022
@antoinerey antoinerey marked this pull request as ready for review May 18, 2022 13:22
@antoinerey
Copy link
Contributor Author

antoinerey commented May 18, 2022

I guess we're good now. 🤗

  • Dot syntax allows any ViteNodeServer option to be set.
  • Properties supporting RegExp must be processed to convert string inputs back to RegExps.
  • Using TS, we make sure to raise a warning when a property supporting RegExps is added but not transformed yet.

@antfu
Copy link
Member

antfu commented May 29, 2022

Renamed to --options for simplicity. Thanks

@antfu antfu changed the title feat(vite-node): Add support for ViteNodeServerOptions (fixes #1208) feat(vite-node): options via CLI (fixes #1208) May 29, 2022
@antfu antfu enabled auto-merge (squash) May 29, 2022 07:52
@antfu antfu disabled auto-merge May 29, 2022 08:24
@antfu antfu merged commit 5deb246 into vitest-dev:main May 29, 2022
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.

Recent versions of vite-node fail to execute a script when ESM is imported
3 participants