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: worker format with worker type #8488

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Jun 7, 2022

Description

fix: #8466

Additional context

  1. support import worker from worker.js?worker&format=esm
  2. support import worker from worker.js?worker&format=iife
  3. worker query dts

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.

@poyoho poyoho requested review from bluwy and removed request for bluwy June 7, 2022 11:38
@patak-dev
Copy link
Member

I fear this may be going too far. Is there a way to support the use case without exposing this?
Maybe there could be an option that instead of generating only one format for the workers, it will start generating both es and iife and automatically load the right one depending on the browser? Kind of like plugin-legacy but for workers.

@poyoho
Copy link
Member Author

poyoho commented Jun 8, 2022

@patak-dev is need to output two format chunks? In fact, two workers can be exported, and users can choose what they like. The remaining workers will generate files but have no code at runtime(clean it by tree-sharking)

// when config `worker.format: all`
export function esmWorkerWrapper() {
  return new Worker(url)
}

export function iifeWorkerWrapper() {
  return new Worker(url2)
}

The disadvantage is that it need to build a worker twice and emit the unused worker

@poyoho
Copy link
Member Author

poyoho commented Jun 8, 2022

I think the url options we can replace like this

import worker, { url } from "./worker.js?worker"

worker.js?worker

export const url = "..."
export default function WorkerWrapper() {
    return new worker("url", {type: "module"})
}

@poyoho
Copy link
Member Author

poyoho commented Jun 8, 2022

And use this plan to judge whether the worker needs to be an inline worker #7352 (comment)

@poyoho poyoho marked this pull request as draft June 8, 2022 12:23
@patak-dev
Copy link
Member

Would you expand a bit more on the proposed changes in your last comments?

@poyoho
Copy link
Member Author

poyoho commented Jun 8, 2022

now I think we can remove all current url query, and then make the content of the url query export by default. (rollup tree-sharking can remove the extra content.)

  1. about url we can export direct. Because it has no extra performance overhead.
  2. about inline worker we can export it by base64 if it filesize less than worker chunk limit and format by iife. or not we can export it default.
    Because it may have a little extra performance overhead. (sourcemap and base64 strings)
  3. about differe worker format differe feature, I prefer to specify to the user. It may be that I don't fully understand what your suggest.

I think the url query syntax we provide is best to have only one level, and the others are exported by default through the virtual module. now it seems just worker work combine multiple query options. 😁

@patak-dev
Copy link
Member

Oh, I see, that is an interesting idea. Maybe something to explore in v4 once we also remove the query in favor of as 'worker'? Something that I like about it is that as only lets you specify one string and not more options, so it may be a way for us to avoid that.
This should also be used not only for workers, and I don't know if it is always possible though. For example in CSS, the ?inline will avoid the side effect of adding the styles so you can't do the same trick there, but maybe that is a special case. It also feels a bit strange that we would potentially allow users to import the worker, the url, and the inline string all at once.

@poyoho
Copy link
Member Author

poyoho commented Jun 8, 2022

I think index.css will like this.

export const inline = 'css code'
export default 'css url'

in v2 we import the internal module like

import indexCSSURL from "./index.css"
import indexCSSRaw from "./index.css?inline"

import worker from "./worker?worker"
import workerURL from "./worker?worker&url"
import inlineWorker from "./worker?worker&inline"

in v3

import indexCSSURL ,{ inline as indexCSSRaw } from "./index.css"
import { url as workerURL, inline }, worker from "./worker?worker"

However, a big problem is that these strings are stored in memory, which may occupy a very large memory

@patak-dev
Copy link
Member

import indexCSSURL ,{ inline as indexCSSRaw } from "./index.css"

This one won't work because when you use the ?inline you are preventing the side-effect of automatically applying the style to the app.

@poyoho
Copy link
Member Author

poyoho commented Jun 8, 2022

oh, I see. ?inline makes it a virtual module. I think this method can reduce the request parameters of the virtual module

@poyoho
Copy link
Member Author

poyoho commented Jun 8, 2022

But now it seems that url, raw, inline are virtual modules based on vite, and they remain as they are, which is easier for users to accept. ?format is an unique to workers that can be confusing.

IIUC, your suggestion is according to the worker type we collected to interpret the format mode.
type: module -> format: esm. type: classic -> format: iife.

@poyoho poyoho marked this pull request as ready for review June 8, 2022 15:58
@poyoho poyoho changed the title feat: worker format with query params feat: worker format with worker type Jun 8, 2022
@rockwotj
Copy link
Contributor

Related here is: #7163

I'd love the ability to just get the URL and format for a another bundle. The annoying thing about the current v2 API today is that I cannot easily reuse it for a service worker, because the URL is hidden.

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.

feat: support generating multiple difference versions of worker files e.g. iife and esm
4 participants