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

refactor(ember): Use @embroider/macros instead of runInDebug for @sentry/ember #2873

Merged
merged 3 commits into from
Oct 6, 2020

Conversation

mydea
Copy link
Member

@mydea mydea commented Sep 2, 2020

This ensures that the debug code is actually stripped from production builds, to produce (slightly, but still) smaller production builds.

Small explanation, if you are not aware of @embroider/macros: This is a purely build-time addon that will strip "dead" code branches of conditions using macroCondition().

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@mydea mydea requested a review from kamilogorek as a code owner September 2, 2020 07:27
@kamilogorek
Copy link
Contributor

cc @k-fish

@kamilogorek kamilogorek requested review from k-fish and removed request for kamilogorek September 2, 2020 09:15
@k-fish
Copy link
Member

k-fish commented Sep 2, 2020

Thanks for the catch!

Ah that's interesting that it doesn't get stripped despite the docs saying it does here: https://github.com/emberjs/ember.js/blob/29accc754fc05a80c236dfcd75abaf08c45514b7/packages/%40ember/debug/index.ts#L267, I see you've brought it up on the ember.js repo, but maybe before it's fully removed that doc can be changed?

I'm going to hold off merging this in until the ember tests are re-enabled on PR's (which should be soon-ish).

@mydea
Copy link
Member Author

mydea commented Sep 3, 2020

Good point, I made a PR to update the docs (emberjs/ember.js#19126).

@mydea mydea force-pushed the ember-improvements branch from f22b5ba to 68eb8af Compare September 24, 2020 08:58
@mydea mydea changed the title Use @embroider/macros instead of runInDebug for @sentry/ember refactor(ember): Use @embroider/macros instead of runInDebug for @sentry/ember Sep 24, 2020
@mydea
Copy link
Member Author

mydea commented Sep 24, 2020

I have updated the PR to also use @embroider/macros to test if in test environment, instead of Ember.testing.

"@sentry/browser": "5.24.2",
"@sentry/tracing": "5.24.2",
"@sentry/types": "5.24.2",
"@sentry/utils": "5.24.2",
"@types/babel__core": "^7.1.9",
Copy link
Member

@k-fish k-fish Sep 24, 2020

Choose a reason for hiding this comment

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

Why did you add babel__core?

Copy link
Member Author

@mydea mydea Sep 25, 2020

Choose a reason for hiding this comment

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

Hmm it resulted in type errors without it, but I just checked again, not anymore! But I have updated @embroider/macros a few versions in between, that probably improved somewhat there. I removed it!

@mydea mydea force-pushed the ember-improvements branch from 68eb8af to f464cac Compare September 25, 2020 07:01
Copy link
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

Thanks!

@k-fish k-fish merged commit 8f66ed8 into getsentry:master Oct 6, 2020
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.

3 participants