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

fix: unexpected temporary file #13269

Merged
merged 1 commit into from
Jun 9, 2023
Merged

Conversation

s10y10
Copy link
Contributor

@s10y10 s10y10 commented May 19, 2023

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.

Review PR in StackBlitz Codeflow Submitted with StackBlitz Codeflow.

@stackblitz
Copy link

stackblitz bot commented May 19, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@s10y10 s10y10 changed the title fix: Unexpected temporary file #13267 fix: unexpected temporary file May 19, 2023
Comment on lines 1096 to 1102
// 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), () => { });
})
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@patak-dev
Copy link
Member

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.

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label May 22, 2023
@patak-dev patak-dev added this to the 4.4 milestone May 22, 2023
patak-dev
patak-dev previously approved these changes May 22, 2023
bluwy
bluwy previously approved these changes May 23, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

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

@dominikg
Copy link
Contributor

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-dev
Copy link
Member

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
Copy link
Member

bluwy commented May 26, 2023

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-dev
Copy link
Member

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
Copy link
Member

bluwy commented May 27, 2023

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
Copy link

silverwind commented Jun 1, 2023

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
Copy link
Contributor Author

s10y10 commented Jun 2, 2023

I also tried using VM to solve it, but I don't know what to do.
[email protected] 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-dev
Copy link
Member

@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
Copy link

silverwind commented Jun 2, 2023

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-dev
Copy link
Member

@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
Copy link
Contributor Author

s10y10 commented Jun 7, 2023

@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-dev
Copy link
Member

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

@s10y10 s10y10 dismissed stale reviews from bluwy and patak-dev via b564c9d June 7, 2023 09:27
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Let's give this a go!

@OllysCoding
Copy link

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
Copy link

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

@VillainsRule
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
8 participants