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

[core-amqp] move to shared rollup config #19588

Merged
merged 3 commits into from
Jan 4, 2022

Conversation

jeremymeng
Copy link
Member

@jeremymeng jeremymeng commented Dec 29, 2021

  • Use the shared rollup config from dev-tool for product.

    while working on this, move StandardAbortMessage from error.ts to
    util/constants.ts to break the circular dependency of errors -> utils
    -> errors.

  • Remove rollup.base.config.js while keeping rollup.test.config.js. Core-amqp has some dependencies that we don't want to add into the shared rollup config so we take an approach similar to what has been done to Event Hubs and Service Bus tests.

Resolves #17808

- Use the shared rollup config from dev-tool for product.

  while working on this, move `StandardAbortMessage` from error.ts to
  util/constants.ts to break the circular dependency of errors -> utils
  -> errors.
while keeping rollup.test.config.js. Core-amqp has some dependencies that we
don't want to add into the shared rollup config so we take an approach similar
to what has been done to Event Hubs and Service Bus tests.
// fs, net, and tls are used by rhea and need to be shimmed
// dotenv doesn't work in the browser, so replace it with a no-op function
shim({
dotenv: `export function config() { }`,
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 browser tests should not have dotenv uses that need to be shimmed

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. We actually don't use env vars at all in any of the tests. Removing.

Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

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

Looks good! I have one comment on shimming dotenv.

@jeremymeng jeremymeng merged commit 2736006 into Azure:main Jan 4, 2022
@jeremymeng jeremymeng deleted the core/amqp-shared-rollup-config branch January 4, 2022 19:30
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.

core-amqp - use shared roll up config from dev-tool
2 participants