[rollup] - Fix bug in circular dependency warning suppression #19012
Conversation
ghost
added
App Configuration
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - event-hubs - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - app-configuration - tests |
maorleger
requested review from
AlonsoMondal,
arifibrahim4,
AriZavala2,
danielav7,
deyaaeldeen,
ellismg,
HarshaNalluru,
jeremymeng,
LuChen-Microsoft,
miguhern,
mikeharder,
praveenkuttappan,
ramya-rao-a,
richardpark-msft,
RoyHerrod,
sarangan12,
whisper6284,
witemple-msft and
xirzec
as code owners
| @@ -125,7 +125,7 @@ export function makeBrowserTestConfig(): RollupOptions { | |||
| // Chai's strange internal architecture makes it impossible to statically | |||
| // analyze its exports. | |||
| chai: ["version", "use", "util", "config", "expect", "should", "assert"], | |||
| ...openTelemetryCommonJs() | |||
| events: ["EventEmitter"] | |||
There was a problem hiding this comment.
I think the definition of openTelemetryCommonJs should be deleted too if it is no longer needed.
I wonder who uses events.
There was a problem hiding this comment.
events started out in the template package and my guess is that it got copied around :)
There was a problem hiding this comment.
We're close to being able to remove this crud - I just know that every rollup file imports this method and uses it, so rather than increasing the PR size I opted to leave the definiton. But I will log a separate issue for me to remove it (here and everywhere that has it)
| if (isNode) { | ||
| dotenv.config(); | ||
| } |
There was a problem hiding this comment.
NIT: I wonder if we need to care about future-proofing tests to accommodate the scenario where a new test suite is added that does not use the recorder but still needs to load environment variables. No need to address this in anyway in this PR though.
What
Why
This is a longstanding issue that we have, where an incorrect logic was copy-pasted to other places. I figured while cleaning this
up that any package I touch can just convert over to the shared dev-tool configuration. Where I was unable to do that, I just
fixed this bug to avoid too many changes in one PR.
Fixes #14292
Resolves #17818
Resolves #17816
Resolves #17815
Resolves #17814
Resolves #17813
Resolves #17810