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: objurl for type module, and concurrent tests #8541

Merged
merged 21 commits into from
Jun 13, 2022

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Jun 11, 2022

Description

  1. trigger worker results directly
  2. revert the worker type, when using .createObjectURL(blob); like https://github.com/vitejs/vite/blob/v2/packages/vite/src/node/plugins/worker.ts#L257

Additional context

About https://github.com/vitejs/vite/runs/6692730196?check_suite_focus=true.Use different subfolders for different builds to avoid deleting another test's folder when other tests are built

the objurl does not seem to run under type: module, but Base64 seems to work, so rolled back the logic to the original version.

About https://github.com/vitejs/vite/runs/6848192738?check_suite_focus=true. The previous unit test waited for about a second, but I found that this error does not necessarily occur. I think it is possible that the loading is completed after 1s, so the waiting time is increased by 1s. I re-run ci 5 times https://github.com/vitejs/vite/runs/6850726819?check_suite_focus=true. the problem is seems no re-play. If there is a better way please feel free to point it out


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 Commit 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.

@patak-dev
Copy link
Member

Would you explain the rationale for reverting the worker type?

Seems the error is still there in CI, https://github.com/vitejs/vite/runs/6843438966?check_suite_focus=true#step:9:80. Hopefully, it is more reproducible. It is strange that this happens only in darwin. This may not be a bug in Vite, but another Node or Vitest issue.

@poyoho
Copy link
Member Author

poyoho commented Jun 11, 2022

After testing, I found that the objurl does not seem to run under type: module, but Base64 seems to work, so I rolled back the logic to the original version.

@sapphi-red
Copy link
Member

const js = 'self.onmessage = () => self.postMessage("foo")'
const blob = new Blob([js], { type: "text/javascript;charset=utf-8" })
const url = URL.createObjectURL(blob)
const w = new Worker(url, { type: 'module' })
URL.revokeObjectURL(url)
w.onmessage = (e) => console.log(e.data)
w.onerror = (e) => console.error(e)
w.postMessage('')
const js = 'self.onmessage = () => self.postMessage("foo")'
const w = new Worker('data:text/javascript;charset=utf-8,' + encodeURI(js), { type: 'module' })
w.onmessage = (e) => console.log(e.data)
w.onerror = (e) => console.error(e)
w.postMessage('')

First one does not work and second one works on Chrome. 🤔

@poyoho
Copy link
Member Author

poyoho commented Jun 13, 2022

yes, so revert the logic. one case just run in classic worker. And second may be run in classic worker and module worker?

@sapphi-red
Copy link
Member

sapphi-red commented Jun 13, 2022

Maybe it is easier to understand if the code is changed like below because Blob URL is only required for Safari (#3468).

  1. try with data url (safari does not work)
  2. try with blob url (chrome + module worker does not work)

Wait, does blob url work with safari + module worker?
If it does not work, I think we need to seek for another method or show a warning.

@patak-dev
Copy link
Member

patak-dev commented Jun 13, 2022

@poyoho I removed the dep until we figure out what is going out with darwin so we avoid confusing other contributors, leaving a note here so we remember to add this back:

Edited: there was an issue with my test, this is a regression, I checked that this works when adding optimizeDeps.disabled: 'build', so looks like this is an issue introduced by the esbuild deps optimization during build.

@poyoho
Copy link
Member Author

poyoho commented Jun 13, 2022

Wait, does blob url work with safari + module worker?

yes, it can work in safari 😁

@poyoho
Copy link
Member Author

poyoho commented Jun 13, 2022

inline type browser worker type res
dataURL chrome module y
dataURL chrome classic y
dataURL safari module y
dataURL safari classic y
blobURL chrome module n
blobURL chrome classic y
blobURL safari module y
blobURL safari classic y

Might it be better to use dataurl directly? @sapphi-red

@patak-dev
Copy link
Member

patak-dev commented Jun 13, 2022

@poyoho I removed the dep until we figure out what is going out with darwin so we avoid confusing other contributors, leaving a note here so we remember to add this back:

Edited: there was an issue with my test, this is a regression, I checked that this works when adding optimizeDeps.disabled: 'build', so looks like this is an issue introduced by the esbuild deps optimization during build.

For reference, we found out why the worker tests were failing after:

We now have deps caches during build that can't be used in parallel. @poyoho manually configured the worker (and assets) playground to have different caches (see c9b185c), so tests are now stable again.

We discussed that we need to implement something here because before building in parallel was possible. One possible way forward is to add a hash to the build deps cache based on the outputDir, but we may need to think if this is enough.

@patak-dev patak-dev changed the title test: trigger worker results directly fix: objurl for type moduletest, and concurrent tests Jun 13, 2022
@patak-dev patak-dev changed the title fix: objurl for type moduletest, and concurrent tests fix: objurl for type module, and concurrent tests Jun 13, 2022
@patak-dev patak-dev merged commit 26ecd5a into vitejs:main Jun 13, 2022
@poyoho poyoho deleted the test/worker-btn branch June 13, 2022 17:38
@sapphi-red
Copy link
Member

Might it be better to use dataurl directly? @sapphi-red

@poyoho I think it is safe to leave blobURL fallback, but it is better to try dataURL first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants