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 Deno detected as a browser in core-rest-pipeline #27090

Conversation

adamcohenhillel
Copy link

Packages impacted by this PR

core-rest-pipeline

Issues associated with this PR

#27077

Describe the problem that is addressed by this PR

Using Azure SDK in a Deno environment causes the following issue:

Error: proxyPolicy is not supported in browser environment
    at https://esm.sh/v132/@azure/core-rest-pipeline@1.12.0/deno/core-rest-pipeline.mjs:2:11023

After investigating a little bit, The following PR might be the thing that triggered it: #26897 - however, it just revealed the underlying issue of - not adding the right policies to isDeno environments

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • [] Added a changelog (if necessary)

@github-actions github-actions bot added Azure.Core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Sep 12, 2023
@github-actions
Copy link

Thank you for your contribution @adamcohenhillel! We will review the pull request and get back to you soon.

pipeline.addPolicy(tlsPolicy(options.tlsOptions));
}

if (isNode || isDeno) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the problem is that Deno is reading the browser key in package.json and loading the proxyPolicy.browser.ts version instead of the node version rather than this policy not being included. After all, if isNode was false, the policy wouldn't be getting pulled in at all rather than loading in and then throwing an error.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be if (isNode && !isDeno)

Copy link
Author

Choose a reason for hiding this comment

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

Mmmm, interesting - so you're saying isNode is true in the Deno environment?

Copy link
Author

Choose a reason for hiding this comment

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

I just tested, and isNode is false in the Deno environment (process is undefined)

image

Copy link
Member

Choose a reason for hiding this comment

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

So if it's false, how is the policy being loaded and throwing an exception for you?

Copy link
Author

@adamcohenhillel adamcohenhillel Sep 12, 2023

Choose a reason for hiding this comment

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

As far as I can tell, the exception is being thrown from the module proxyPolicy.browser.ts and not the module proxyPolicy.ts - which is the one that is being imported here in this file, and where proxyPolicy() is declared.

It looks confusing, as both modules have proxyPolicy() function. So I guess the other proxyPolicy is being used as the default

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but even if it's loading the browser version, it has to be called in order to throw, which means it has to be added to the pipeline. So if the existing check for isNode is false, how is it being added?

Is another dependency perhaps shimming out a process global object in your bundle?

Copy link
Author

Choose a reason for hiding this comment

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

So core-rest-pipeline is not a direct import of my code. It's a dependency of @azure/openai@1.0.0-beta.4 - so maybe something in that package changes the global object..?

This is my full code:

import "https://deno.land/x/xhr@0.2.1/mod.ts";
import { serve } from "https://deno.land/std@0.170.0/http/server.ts";
import { OpenAIClient, AzureKeyCredential } from "https://esm.sh/@azure/openai@1.0.0-beta.4";

import { corsHeaders } from "../common/cors.ts";

const serviceFun = async (req: Request) => {
  try {
    if (req.method === "OPTIONS") {
      return new Response("ok", { headers: corsHeaders });
    }

    const azureAPIKey = Deno.env.get("AZURE_OPENAI_API_KEY");
    const openaiAzure = new OpenAIClient("https://XXX.openai.azure.com/", new AzureKeyCredential(azureAPIKey));
    completion = await openaiAzure.getChatCompletions("deploymentName", [{ role: "system", content: prompt }]);

    return new Response(JSON.stringify({ msg: completion.content }), {
      status: 200,
      headers: { ...corsHeaders, "Content-Type": "application/json" },
    });
  } catch (err: unknown) {
    console.log("!!! Error: ", err);
  }
};

serve(serviceFun);

I tried to look into how Azure openai sdk usescore-rest-pipeline , and it is too lacking (see https://github.com/search?q=repo%3AAzure%2Fazure-sdk-for-js+core-rest-pipeline+path%3Aopenai&type=code) -
Honestly I don't know..

Copy link
Author

Choose a reason for hiding this comment

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

@samipsj
Copy link

samipsj commented Sep 28, 2023

Hey guys, any workaround for this until its fixed?
Was using https://esm.sh/@azure/ai-form-recognizer@4.0.0 for deno import
Everything was working fine.. redeployed the edge function now getting the same error mentioned in this issue.
(supabase edge function)

@xirzec
Copy link
Member

xirzec commented Oct 4, 2023

I looked into this a little more, the problem is that esm.sh is polyfilling the node process object such that our isNode check is true, even though esbuild is compiling the browser version:

import { isNode } from "https://esm.sh/@azure/core-util@1.4.0/";

console.log(`isNode: ${isNode}`);
image

It looks like Deno's standard library is spoofing itself as Node now:

https://deno.land/std@0.177.0/node/process.ts?s=versions

This is reminding me horribly of browsers doing User-Agent sniffing. Perhaps we need to make our isNode check have a !isDeno or something? @mpodwysocki

@adamcohenhillel
Copy link
Author

Hey @xirzec - did you get around to doing that? happy to change my PR with your suggestions if not

@xirzec
Copy link
Member

xirzec commented Oct 18, 2023

Hey @xirzec - did you get around to doing that? happy to change my PR with your suggestions if not

I threw out a new PR here: #27459

xirzec added a commit that referenced this pull request Oct 18, 2023
)

### Packages impacted by this PR

`@azure/core-util`

### Issues associated with this PR

Fixes #27077

### Describe the problem that is addressed by this PR

Deno implemented `process.versions.node`, which is how we detect that we
are running inside a Node environment. Since Deno actually is much
closer to a browser environment, this is *not* what was expected at
runtime and resulted in things like loading the browser version of
pipeline policies that throw due to not being implemented/relevant in
browsers.

I kept this really simple by making `isNode` check that `isDeno` is
false. Since `isNode` was previously false inside Deno environments, I
don't think this will break anything.

### Provide a list of related PRs _(if any)_

#27090
@xirzec
Copy link
Member

xirzec commented Oct 19, 2023

Closing this one since #27459 went in

@xirzec xirzec closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants