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

add module upload support #1010

Merged
merged 2 commits into from
Aug 11, 2022

Conversation

jgodson
Copy link
Contributor

@jgodson jgodson commented Aug 2, 2022

PR to upstream the changes from Shopify#16 that adds support for uploading scripts that use the module syntax.

It builds upon the changes started here

Closes #794

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

changelog detected ✅

@sodabrew
Copy link
Contributor

sodabrew commented Aug 2, 2022

Awesome! Don't say I never shopped on Shopify :)

Since I'm just working on a round-trip use case over in #1007, I'll note that DownloadWorker also needs to handle module-format workers. There is only an example for a plain worker at https://api.cloudflare.com/#worker-script-download-worker

$ curl <auth headers> https://api.cloudflare.com/client/v4/accounts/<accountid>/workers/scripts/<scriptname>

Content-Type: application/json
[rest of the headers]

self.addEventListener... [rest of the script]

The same API call for a Module-format worker returns multipart that would need to be parsed. What should happen if there are multiple parts?

$ curl <auth headers> https://api.cloudflare.com/client/v4/accounts/<accountid>/workers/scripts/<scriptname>

Content-Type: multipart/form-data; boundary=abcdefg
[rest of the headers]

--abcdefg
Content-Disposition: form-data; name="worker.js"

export default {
  fetch(request) {
    return new Response("Hello world")
  },
};
--abcdefg--

The return type from cloudflare.DownloadWorker is

type WorkerScriptResponse struct {
        Response
        WorkerScript `json:"result"`
}

But the code for the method actually does this:

...
        var r WorkerScriptResponse
        r.Script = string(res)
        r.Success = true
        return r, nil
}

I'm not sure I follow what's going on there with how Go is inferring and passing types from that method (limitations of my Go knowledge)

@sodabrew
Copy link
Contributor

sodabrew commented Aug 3, 2022

Got it, the struct fields are being promoted from nested structs

type WorkerScriptResponse struct {
        Response
        WorkerScript `json:"result"`
}

type WorkerScript struct {
        WorkerMetaData
        Script     string `json:"script"`
        UsageModel string `json:"usage_model,omitempty"`
}

type WorkerMetaData struct {
        ID         string    `json:"id,omitempty"`
        ETAG       string    `json:"etag,omitempty"`
        Size       int       `json:"size,omitempty"`
        CreatedOn  time.Time `json:"created_on,omitempty"`
        ModifiedOn time.Time `json:"modified_on,omitempty"`
}

Thing is, most of these fields are not set by the DownloadWorker response!

So back to the issue at hand: is it possible for a DownloadWorker to respond with multiple module files? If so, what's the best way to represent that as a Go structure? (I'm thinking map[string]WorkerScript where the key is the module file name)

@jgodson
Copy link
Contributor Author

jgodson commented Aug 3, 2022

Is it possible for a DownloadWorker to respond with multiple module files?

That is an excellent question. I'm not really sure as I haven't tried that/needed to do that yet.

I believe that the upload functionality at least is covered here, so if download could be implemented separately, this should be good to go as is (at least it has been working for us so far).

@jacobbednarz
Copy link
Member

@jgodson i'm hoping to getting to review/test this in the coming days. are you able to address the CHANGELOG entry so there is one less thing to do before we merge?

@jacobbednarz jacobbednarz merged commit 81c1f2f into cloudflare:master Aug 11, 2022
@github-actions github-actions bot added this to the v0.47.0 milestone Aug 11, 2022
github-actions bot pushed a commit that referenced this pull request Aug 11, 2022
@jacobbednarz
Copy link
Member

thanks for your patience here @jgodson. the PR looks great and my (basic) testing of this internally is looking great.

@sodabrew
Copy link
Contributor

@jacobbednarz Glad to see this land! Where are you at in terms of thinking about how to solve for DownloadWorker to return something sensible when a module worker is fetched? #1010 (comment)

@jacobbednarz
Copy link
Member

haven't really dug into the issues with DownloadWorker and i'd have to chat with the team about how they intend to solve it for multiple workers.

@github-actions
Copy link
Contributor

This functionality has been released in v0.47.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@jgodson jgodson deleted the add-module-upload-support branch June 13, 2023 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for UploadWorker as a module
4 participants