Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.06k stars 1.19k forks source link

Vite load wrong version during browser test when having two different versions(direct + transitive) #30297

Open kazrael2119 opened 3 months ago

kazrael2119 commented 3 months ago

from this https://github.com/Azure/azure-sdk-for-js/pull/30296 , we add @azure/arm-network as the third- party dependency to create a private endpoint in test case. And this will cause browser test failure as no polling for lro cases.

steps:

  1. checkout https://github.com/Azure/azure-sdk-for-js/pull/30296
  2. run rush update && rush build -t @azure/arm-mongocluster
  3. cd sdk/mongocluster/arm-mongocluster
  4. run rushx unit-test:browser
qiaozha commented 2 months ago

I don't think it's related to any third party dependency, from bundle a package's perspective, Is there any difference between bundle a core-client depedency or bundle another arm library depedency?

My instinction is we need to look into how we bundle the library if we have two different versions of dependencies to the same library in the browser.

In mongocluster's case, we have two different versions of core-lro, arm-network which is generated from swagger uses core-lro v2, and mongocluster which is generated from Modular uses core-lro v3.

Another case is FaceAI, it also has this problem, and we just ignored it before, their case has multipart form data, which also has two different versions of dependencies to the same library of core-client-rest.

I may be wrong about the root cause, @xirzec @joheredi might have better suggestions on where to look into for this.

But I still like to point out, if my assumption is correct, this will be an important impact to us for management plane adopting to Modular. Because for large RP like compute and network, their adoption to typespec will probably not happen so soon. So for at least a significant amount of time, the two different versions of the same package dependency would be a problem for us. It basically means we are abondoning our browser customers as long as we migrate to Modular for new RP like mongocluster if we can't fix it from the bundle tooling side.

joheredi commented 2 months ago

@MaryGao can you help us investigate this issue. I would like to understand what is the underlying problem. As I understand from the description, adding an additional library (which potentially uses a different version of core-lro) causes existing code to stop working.

Does this reproduce when the test recorder is not involved? (want to scope down the issue, so if we can rule out a proxy/recorder issue, it would be beneficial)

qiaozha commented 2 months ago

@joheredi your understanding is correct, but this is not related to lro, in faceAI's case, it's the core-client-rest that has two different versions. But I do think elimate the test recorder's impact first is a good idea.

MaryGao commented 2 months ago

Does this reproduce when the test recorder is not involved?

Sure I will try this in non-test environemnt and I'd like to mention that this test case is working in node environment. https://github.com/Azure/azure-sdk-for-js/tree/main/sdk/mongocluster/arm-mongocluster/test/public/node

I will take a deep look at the root cause.

glharper commented 2 months ago

I've run into this issue where vitest fails to build for browser. The issue seems to be when @azure-tools/test-credentials is included (for my tests, the addition of the createTestCredential method brought about failure).

From what i can tell, the problem is this line:

// ../../../common/temp/node_modules/.pnpm/@azure-tools+test-recorder@3.5.2/node_modules/@azure-tools/test-recorder/dist-esm/src/utils/env.browser.js
var env = window.__env__;

This appears to be not picking up whatever cli or env vars exist to set TEST_MODE.

MaryGao commented 2 months ago

We verified this is okay in node environement. But for browser environement, I am blocked by the authentication.

If I switched to InteractiveBrowserCredential(doc) it would require a client_id and this violates with our recent policy. @KarishmaGhiya Do we have a way to authenticate ourselves in browser without a client_id?

image image

MaryGao commented 2 months ago

@mpodwysocki @harshanalluru our browser testing failed bundling correct core-lro version when having both v2 and v3 core-lro versions(direct + transitive). Could you help on how to fix this issue?

Background

In our SDK we have two direct dependencies, 1) a direct v3 lro dependency and 2) a dev dependency arm-network which refers another v2 lro.

├─┬ @azure/arm-network
│ ├─┬ @azure/core-lro@2.7.2
├── @azure/core-lro@3.0.0
├── ...

We have a chance to verify following 4 environements. And only the browser testing has issues to create our resource. It seems it is introduced by bundling wrong lro version.

// v2 lro
const client: MongoClusterManagementClient = new MongoClusterManagementClient(credential, subscriptionId, recorder.configureClientOptions({}));
// v3 lro
const networkClient: NetworkManagementClient = new NetworkManagementClient(credential, subscriptionId, recorder.configureClientOptions({}));
// should be v3 but called v2
const poller = await client.mongoClusters.createOrUpdate(...);

Debugging

Here are our debugging snapshots we found the vitest loaded v2(if we have a getResult method which means v2 lro; if we have a submitted method which means v3 lro).

image image

How to repro

  1. checkout https://github.com/Azure/azure-sdk-for-js/pull/30296
  2. cd sdk/mongocluster/arm-mongocluster
  3. run rush update && rush build -t .
  4. run export TEST_MODE=playback && rushx unit-test:browser

/cc @joheredi @qiaozha @xirzec

KarishmaGhiya commented 2 months ago

@MaryGao What is the policy that is getting violated by passing in the client id for IBC? I don't think I understand why that is a problem.

client id is public information, it's not a secret.

HarshaNalluru commented 2 months ago

@MaryGao @kazrael2119

From what we have observed, vitest does prebundling and seems to be messing with the dependency linking. If you face this issue for any of the packages, follow the https://github.com/Azure/azure-sdk-for-js/commit/488cc51 change.

Update being: optimizeDeps.include: [ "@azure/core-lro" ] in the vitest browser config.

HarshaNalluru commented 2 months ago

I was talking to @xirzec and we were thinking of mentioning all the core packages in the root vitest config for browsers under this field.

@MaryGao You can unblock yourself with this fix for now. I'll keep you posted when we tweak the root configs.

qiaozha commented 2 months ago

@HarshaNalluru I am a little curious, is it a temporary solution ? I guess this problem does not only exist for azure core related packages? what if there's other external dependencies that we have different versions relied with ?

I wonder if we could try to resolve this issue from vitest prebundling side.

kazrael2119 commented 2 months ago

@MaryGao @kazrael2119

From what we have observed, vitest does prebundling and seems to be messing with the dependency linking. If you face this issue for any of the packages, follow the 488cc51 change.

Update being: optimizeDeps.include: [ "@azure/core-lro" ] in the vitest browser config.

if use this, core-lro will always be v3 which can call the v3 cases(arm-mongocluster) but fail to call the v2 cases(arm-network)