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

Provide an optional argument for loadConfigFromFile() to avoid leaving temporary file in user workspace #15441

Closed
4 tasks done
zhangyx1998 opened this issue Dec 27, 2023 · 3 comments
Labels
duplicate This issue or pull request already exists

Comments

@zhangyx1998
Copy link

zhangyx1998 commented Dec 27, 2023

Description

Although current implementation is already invulnerable to exceptions from the imported userConfig (using finally statememt), it can still leave the temporary file undeleted when user hits CTRL-C on server start (this happens randomly)

See this issue: vuejs/vitepress#3382

Suggested solution

Provide an additional optional parameter to specify where to save the temporary file so downstream frameworks can save temp file to their cache folder.

Example:

export async function loadConfigFromFile(
  configEnv: ConfigEnv,
  configFile?: string,
  configRoot: string = process.cwd(),
  logLevel?: LogLevel,
  tempDir?: string = configRoot, // 👈 suggested change
): Promise<{
  path: string
  config: UserConfig
  dependencies: string[]
} | null> { /* ... */ }

Alternative

  1. Instead of writing the file to disk and then importing it back dynamically, transform the code into an IIFE and use eval() to run it.

  2. Mask SIGINT before import and unmask it in the finally block.

Additional context

As discussed in the issue (link above), moving bundled file to a different path could mess up relative imports (or requires).

However, it seems like the bundling process always produces a monolithic file without any external dependency (as it is supposed to). My theory is, as long as the process's current working directory doesn't change, nothing should break.

Validations

@zhangyx1998 zhangyx1998 changed the title Provide an optional argument for loadConfigFromFile() to avoid leaving cached temporary file in user workspace Provide an optional argument for loadConfigFromFile() to avoid leaving temporary file in user workspace Dec 27, 2023
@sapphi-red
Copy link
Member

We cannot just write to a different directory otherwise the relative/bare imports will be resolved to a different file.
There an issue about the temporary file and I believe solving the issue rather than adding an option is better.
See #13269 and #13267 (comment) for the idea of using data: import specifier.

@sapphi-red sapphi-red closed this as not planned Won't fix, can't repro, duplicate, stale Dec 30, 2023
@sapphi-red sapphi-red added duplicate This issue or pull request already exists and removed enhancement: pending triage labels Dec 30, 2023
@zhangyx1998
Copy link
Author

zhangyx1998 commented Dec 30, 2023

We cannot just write to a different directory otherwise the relative/bare imports will be resolved to a different file. There an issue about the temporary file and I believe solving the issue rather than adding an option is better. See #13269 and #13267 (comment) for the idea of using data: import specifier.

Thanks for the information!

BTW, may I ask about the rationale of bundling user config? That is, if user config is also ESM, why not use dynamic import directly?

@sapphi-red
Copy link
Member

BTW, may I ask about the rationale of bundling user config? That is, if user config is also ESM, why not use dynamic import directly?

It is to collect the dependency information (#5779).

@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants