fix: unexpected temporary file by s10y10 · Pull Request #13269 · vitejs/vite

s10y10 · 2023-05-19T09:29:32Z

Description

Fixes #13267
Fixes #9470

Check and clean temporary files every time loadConfigFromBundledFile is executed.

Additional context

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the PR Title Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

Submitted with StackBlitz Codeflow.

bolt-new-by-stackblitz · 2023-05-19T09:29:34Z

Run & review this pull request in StackBlitz Codeflow.

patak-cat · 2023-05-19T09:47:37Z

packages/vite/src/node/config.ts

+    // check and clear legacy Tmp files
+    const fileBaseName = path.basename(fileName)
+    const fileDir = path.dirname(fileName)
+    const timestampFiles = (await fsp.readdir(fileDir)).filter(name=>name.includes(`${fileBaseName}.timestamp-`))
+    timestampFiles.forEach(name=>{
+      fs.unlink(path.resolve(fileDir,name), () => { });
+    })


Two vite processes may be executed using different cache dirs on the same folder at the same time so this may delete a file while it is being used. Maybe we should move the generation of the file to be in ${cacheDir}/temp/${fileNameTmp} so we don't need the clean-up.

That's a good idea. I'll try to make some modifications.

When I tried to move the file generation to the ${cacheDir} directory, I found that the value of cacheDir could not be obtained in the loadConfigFromBundledFile method. Therefore, I considered whether it was possible to import directly in base64 format without using a temporary file

packages/vite/src/node/config.ts

patak-cat · 2023-05-22T08:48:23Z

I think this could work better! I don't see limits affecting using base64 to import the file. Let's queue this for Vite 4.4 just to be on the safe side, and also to give others the opportunity to review the new approach.

bluwy

This is actually pretty smart. Looking forward to testing this out.

dominikg · 2023-05-26T11:21:28Z

are there any downsides to this like extra memory overhead for the b64 string or super strange errors that reference it instead of the config file?

patak-cat · 2023-05-26T14:55:36Z

That's a good point @dominikg

This is how errors look right now in main:

failed to load config from /Users/patak/test/vite-project/vite.config.ts
error during build:
ReferenceError: test is not defined
    at file:///vite-project/vite.config.ts.timestamp-1685112501902-8acd10cb0069.mjs:3:1
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

This is the same error after this PR:

failed to load config from /Users/patak/test/vite-project/vite.config.ts
error during build:
ReferenceError: test is not defined
    at data:text/javascript;base64,Ly8gdml0ZS5jb25maWcudHMKaW1wb3J0IHsgZGVmaW5lQ29uZmlnIH0gZnJvbSAiZmlsZTovLy9Vc2Vycy9wYXRhay92L3ZpdGUtcHIyL3BhY2thZ2VzL3ZpdGUvZGlzdC9ub2RlL2luZGV4LmpzIjsKdGVzdDsKdmFyIHZpdGVfY29uZmlnX2RlZmF1bHQgPSBkZWZpbmVDb25maWcoewogIGJ1aWxkOiB7CiAgICBtaW5pZnk6IGZhbHNlCiAgfQp9KTsKZXhwb3J0IHsKICB2aXRlX2NvbmZpZ19kZWZhdWx0IGFzIGRlZmF1bHQKfTsKLy8jIHNvdXJjZU1hcHBpbmdVUkw9ZGF0YTphcHBsaWNhdGlvbi9qc29uO2Jhc2U2NCxld29nSUNKMlpYSnphVzl1SWpvZ015d0tJQ0FpYzI5MWNtTmxjeUk2SUZzaWRtbDBaUzVqYjI1bWFXY3VkSE1pWFN3S0lDQWljMjkxY21ObGMwTnZiblJsYm5RaU9pQmJJbU52Ym5OMElGOWZkbWwwWlY5cGJtcGxZM1JsWkY5dmNtbG5hVzVoYkY5a2FYSnVZVzFsSUQwZ1hDSXZWWE5sY25NdmNHRjBZV3N2ZEdWemRDOTJhWFJsTFhCeWIycGxZM1JjSWp0amIyNXpkQ0JmWDNacGRHVmZhVzVxWldOMFpXUmZiM0pwWjJsdVlXeGZabWxzWlc1aGJXVWdQU0JjSWk5VmMyVnljeTl3WVhSaGF5OTBaWE4wTDNacGRHVXRjSEp2YW1WamRDOTJhWFJsTG1OdmJtWnBaeTUwYzF3aU8yTnZibk4wSUY5ZmRtbDBaVjlwYm1wbFkzUmxaRjl2Y21sbmFXNWhiRjlwYlhCdmNuUmZiV1YwWVY5MWNtd2dQU0JjSW1acGJHVTZMeTh2VlhObGNuTXZjR0YwWVdzdmRHVnpkQzkyYVhSbExYQnliMnBsWTNRdmRtbDBaUzVqYjI1bWFXY3VkSE5jSWp0cGJYQnZjblFnZXlCa1pXWnBibVZEYjI1bWFXY2dmU0JtY205dElDZDJhWFJsSjF4dVhHNTBaWE4wSUZ4dVhHNWxlSEJ2Y25RZ1pHVm1ZWFZzZENCa1pXWnBibVZEYjI1bWFXY29lMXh1SUNBZ0lHSjFhV3hrT2lCN1hHNGdJQ0FnSUNBZ0lHMXBibWxtZVRvZ1ptRnNjMlZjYmlBZ0lDQjlYRzU5S1RzaVhTd0tJQ0FpYldGd2NHbHVaM01pT2lBaU8wRkJRVFJSTEZOQlFWTXNiMEpCUVc5Q08wRkJSWHBUTzBGQlJVRXNTVUZCVHl4elFrRkJVU3hoUVVGaE8wRkJRVUVzUlVGRGVFSXNUMEZCVHp0QlFVRkJMRWxCUTBnc1VVRkJVVHRCUVVGQkxFVkJRMW83UVVGRFNpeERRVUZET3lJc0NpQWdJbTVoYldWeklqb2dXMTBLZlFvPQo=:3:1
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

So if we want to do the change, we'll need to catch the error and rewrite it. Would that still be ok?

bluwy · 2023-05-26T16:22:21Z

As long as we have the failed to load config from /Users/patak/test/vite-project/vite.config.ts part, I think that's fine. The file:///vite-project/vite.config.ts.timestamp-1685112501902-8acd10cb0069.mjs:3:1 part also wasn't as helpful before.

patak-cat · 2023-05-26T18:22:25Z

The :3:1 in this case was the right line/column, but I imagine that is totally unreliable. Maybe we should try to get sourcemaps? Probably better to just remove the extra line in the error as you suggest and try to remove this hack altogether in the future.

bluwy · 2023-05-27T06:40:22Z

I think we can definitely improve those, though editing error stacktraces is always a bit risky. But maybe we can improve that later and get this in first since it helps to remove the temporary file (which I've seen from time-to-time)

silverwind · 2023-06-01T14:25:56Z

I tried this method in an earlier attempt to fix this temp file issue, but ran into the problem of relative import specifiers not working because when node loads a file from a data: url, it has no concept of a working directory and therefore can not resolve relative imports. From node docs:

data: URLs only resolve bare specifiers for builtin modules and absolute specifiers. Resolving relative specifiers does not work because data: is not a special scheme. For example, attempting to load ./foo from data:text/javascript,import "./foo"; fails to resolve because there is no concept of relative resolution for data: URLs.

So I think this method is affected as well. Not that I'd use relative imports in a vite config, but some people surely do.

I think VM modules would be the proper solution.

s10y10 · 2023-06-02T03:08:59Z

I also tried using VM to solve it, but I don't know what to do.
vite@3.1.5 Converted the dependent path in config, and there should be no problem with the relative path. You can try using the latest version #10254

patak-cat · 2023-06-02T08:35:28Z

@silverwind if you can reproduce the issue you mentioned, would you share a reproduction that isn't working with this PR so we have it as a reference here?

silverwind · 2023-06-02T09:11:07Z

If it's true that you rewrite relative paths already and you have test cases confirming relative paths work in both static and dynamic import in the config, it should be fine.

I don't have anything to share, it was just local experimenation that I never commited anywhere.

patak-cat · 2023-06-06T14:34:23Z

@s10y10 let us know if you'd like to work on the error message issue described here. We could work on it if you prefer that.

s10y10 · 2023-06-07T03:33:59Z

@patak-dev I don't know how to rewrite the errorstack in order to get line/column and ensure security...... I may be make modification like this:
try{ //dynamicImport }catch(e){ throw new Error(${e.message} at ${fileName}); }

patak-cat · 2023-06-07T08:20:46Z

@s10y10 I think we don't need to support line/column for now. What you propose sounds good 👍🏼

patak-cat

The error looks good now. I think we could try during the 4.4 beta. @bluwy, what do you think?

bluwy

Let's give this a go!

This reverts commit ff3ce31.

OllysCoding · 2023-07-07T13:17:22Z

With this now reverted do we have any other plans to address the temporary file issue? This completely prevents the ability to use vite on limited filesystems

dylan-mutuals · 2024-06-15T10:31:00Z

@OllysCoding you can workaround it by using a vite.config.cjs file. Not ideal by any means but it works for now.

VillainsRule · 2024-09-03T00:51:55Z

if y'all are still having issues just recode the entire thing...painful but it normally starts whenever you start a project and is hard to miss, idk why it happened but it's not a common bug

If node_modules is not writable, loadConfigFromBundledFile fails to write its temp file and vite aborts with an exception. This was previously fixed in vitejs#13269, but was reverted after several reports of failure to load config with relative imports (see vitejs#13631 and vitejs#13730). Instead of replacing the current code, try to load from string only as a workaround, when the directory doesn't exist. This should solve the case of unwritable node_modules when relative imports are not used without breaking anything that works.

s10y10 changed the title ~~fix: Unexpected temporary file #13267~~ fix: unexpected temporary file May 19, 2023

patak-cat reviewed May 19, 2023

View reviewed changes

s10y10 force-pushed the s10y10/issue13267 branch from 512deef to 9f52e21 Compare May 22, 2023 06:51

patak-cat reviewed May 22, 2023

View reviewed changes

packages/vite/src/node/config.ts Outdated Show resolved Hide resolved

patak-cat added the p2-nice-to-have Not breaking anything but nice to have (priority) label May 22, 2023

patak-cat added this to the 4.4 milestone May 22, 2023

s10y10 force-pushed the s10y10/issue13267 branch from 9f52e21 to 6e43e5d Compare May 22, 2023 10:04

patak-cat previously approved these changes May 22, 2023

View reviewed changes

bluwy previously approved these changes May 23, 2023

View reviewed changes

patak-cat added the on hold label May 26, 2023

patak-cat mentioned this pull request Jun 1, 2023

avoid writing the temporary file when evaluating the config file #9470

Closed

4 tasks

patak-cat linked an issue Jun 1, 2023 that may be closed by this pull request

avoid writing the temporary file when evaluating the config file #9470

Closed

4 tasks

fix: unexpected temporary file (vitejs#13267)

b564c9d

s10y10 dismissed stale reviews from bluwy and patak-cat via b564c9d June 7, 2023 09:27

s10y10 force-pushed the s10y10/issue13267 branch from 6e43e5d to b564c9d Compare June 7, 2023 09:27

patak-cat approved these changes Jun 9, 2023

View reviewed changes

bluwy approved these changes Jun 9, 2023

View reviewed changes

patak-cat merged commit ff3ce31 into vitejs:main Jun 9, 2023

patak-cat mentioned this pull request Jun 13, 2023

fix: timestamp config dynamicImport #13502

Merged

4 tasks

sapphi-red mentioned this pull request Jun 26, 2023

Vite cannot load vite.config.ts with Node 17 and vite@4.4.0-beta.3 #13631

Closed

7 tasks

cpojer mentioned this pull request Jul 6, 2023

[4.4.0] "Invalid URL" error with dynamic imports in plugins. #13730

Closed

7 tasks

patak-cat added a commit that referenced this pull request Jul 6, 2023

Revert "fix: unexpected config temporary file (#13269)"

6389d6d

This reverts commit ff3ce31.

sapphi-red mentioned this pull request Jul 7, 2023

There will be temporary vite.config.js.timestamp files that have not been cleaned up #13267

Closed

7 tasks

sapphi-red mentioned this pull request Dec 30, 2023

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

Closed

4 tasks

hi-ogawa mentioned this pull request Aug 18, 2024

fix: load esm vite config without a temporary file #17894

Closed

4 tasks

sapphi-red mentioned this pull request Oct 30, 2024

fix(config): write temporary vite config to node_modules #18509

Merged

orgads mentioned this pull request Apr 27, 2025

fix(config): workaround failure creating temporary file (take 2) #19952

Open

Uh oh!

Conversation

s10y10 commented May 19, 2023 • edited by patak-cat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Uh oh!

bolt-new-by-stackblitz bot commented May 19, 2023

Uh oh!

patak-cat May 19, 2023

Choose a reason for hiding this comment

Uh oh!

s10y10 May 19, 2023

Choose a reason for hiding this comment

Uh oh!

s10y10 May 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patak-cat commented May 22, 2023

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

dominikg commented May 26, 2023

Uh oh!

patak-cat commented May 26, 2023

Uh oh!

bluwy commented May 26, 2023

Uh oh!

patak-cat commented May 26, 2023

Uh oh!

bluwy commented May 27, 2023

Uh oh!

silverwind commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s10y10 commented Jun 2, 2023

Uh oh!

patak-cat commented Jun 2, 2023

Uh oh!

silverwind commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patak-cat commented Jun 6, 2023

Uh oh!

s10y10 commented Jun 7, 2023

Uh oh!

patak-cat commented Jun 7, 2023

Uh oh!

patak-cat left a comment

Choose a reason for hiding this comment

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

OllysCoding commented Jul 7, 2023

Uh oh!

dylan-mutuals commented Jun 15, 2024

Uh oh!

VillainsRule commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

s10y10 commented May 19, 2023 •

edited by patak-cat

Loading

silverwind commented Jun 1, 2023 •

edited

Loading

silverwind commented Jun 2, 2023 •

edited

Loading