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

Enable partial Ivy format for Angular #5268

Closed
BojanKogoj opened this issue Jun 16, 2022 · 26 comments
Closed

Enable partial Ivy format for Angular #5268

BojanKogoj opened this issue Jun 16, 2022 · 26 comments
Assignees
Labels
Package: angular Issues related to the Sentry Angular SDK Type: Improvement

Comments

@BojanKogoj
Copy link

Problem Statement

Angular 13+ will remind you of any library, that isn't compiled in partial Ivy and requires using ngcc.

Processing legacy "View Engine" libraries:
- @sentry/angular [es2015/esm2015] (git://github.com/getsentry/sentry-javascript.git)
Encourage the library authors to publish an Ivy distribution.

Any chance to go Ivy way? I know some users might not be using Ivy yet, but I'd like to get rid of ngcc as soon as possible.

Solution Brainstorm

In tsconfig remove "enableIvy": false.

@AbhiPrasad
Copy link
Member

Hey, thanks for writing in. We've chatted about this in #5253 (comment), and will be addressing this in our next major version, v8. See #5194 (comment) also.

Backlogging this for now. If anyone else is interested in this (or is interested in having this happen before the next major), please leave a reaction to this issue or a comment in the issue itself.

@henningtillmann
Copy link

Hi! Is there any roadmap or date, when v8 (with Ivy support) will be released?

@AbhiPrasad
Copy link
Member

No timelines for the next major at the moment, cc @vladanpaunovic

@vladanpaunovic
Copy link
Contributor

@henningtillmann, thanks for your interest!

Sadly I don't have dates to share with you yet.

What I can tell you is that we are currently on v7.9.0 and I don't think it will take us long enough before we jump to version 8.

Once we know more, we will update the milestone.

@jpike88
Copy link

jpike88 commented Nov 4, 2022

The version before v7.0.0 was like v6.19.0...

Out of curiosity, how do you decide whether to increment the minor version, versus the patch version? I'm looking at the release history and at a glance it seems inconsistent to me.

@AbhiPrasad
Copy link
Member

@jpike88 we follow semantic versioning, so if the release is fixes only we make it a patch, and if it includes new features we make it a minor. There are exceptions to this rule, but 95% of time we follow this.

@Lonli-Lokli
Copy link

@vladanpaunovic - version 8.0: No due date, 7% complete

Seems like it will not happen in the nearest 6 months, right?

@Lonli-Lokli
Copy link

Angular 15 is already available

@Lms24
Copy link
Member

Lms24 commented Dec 9, 2022

Hi everyone, just wanted to give you an update on this issue:

As I already described in #5253 (comment), we can only make the switch to Ivy in our next major, v8. We're currently trying to find out, how many of our users are still on Angular 10 and 11 to determine, if we can simple bump this package to support Angular 12+, which makes it possible for us to enable Ivy format. In case there's still a considerable amount of NG10 and 11 users, we'll still bump the @senrty/angular package to 12+ and provide a separate "legacy" package for them.

I'm fully aware that many of you want to see this happening sooner (I cannot give an ETA on v8) but for now I'm afraid the warning message will persist. However, we're interested in feedback here:

  • What is the impact of this warning message for you? Do you just want to get rid of it (fully understandable) or is there more serious impact in terms of e.g. performance?
  • If you have experience with Angular library development, is there a way we're not aware of, how we could introduce this change in a non-breaking way? IOW, can we enable Ivy support without impacting Angular 10 and 11 users at all in this very package?
  • If you are still using Angular 11 or older, is it likely that you'll upgrade to a newer version soon? If not, do you expect Sentry to continue support, although LTS support for your Angular version was dropped long ago?

Thanks for your patience and looking forward to reading your feedback.

@Lms24
Copy link
Member

Lms24 commented Dec 9, 2022

Angular 15 is already available

Yup, we're aware and our SDK already supports it since version 7.20.0

@jpike88
Copy link

jpike88 commented Dec 9, 2022

  • What is the impact of this warning message for you? Do you just want to get rid of it (fully understandable) or is there more serious impact in terms of e.g. performance?

Just want to get rid of the warning message, but beyond that it doesn't have any performance impact I'm aware of.

  • If you have experience with Angular library development, is there a way we're not aware of, how we could introduce this change in a non-breaking way? IOW, can we enable Ivy support without impacting Angular 10 and 11 users at all in this very package?

From my understanding, this is only a change that application developers make regarding the AOT compiler and linker, but the ivy engine is the ivy engine. So it should be compatible as far back as v8 (which needed ivy to be opt-in enabled)

@ranma42
Copy link

ranma42 commented Dec 9, 2022

  • What is the impact of this warning message for you? Do you just want to get rid of it (fully understandable) or is there more serious impact in terms of e.g. performance?

IIUC there is a performance impact in terms of the build performance, since view engine libraries require an additional step of analysis/conversion at build time.
The results of this step are cached locally so the impact should be negligible in the developers' environment, but it likely to be present in CI/CD pipelines (unless additional care is taken to manage the cache).

@henningtillmann
Copy link

  • What is the impact of this warning message for you? Do you just want to get rid of it (fully understandable) or is there more serious impact in terms of e.g. performance?

My app is a bigger web component, i.e. bundle size is very important. Every byte saved helps. Therefore, Ivy is a big help and mandatory.

@decline
Copy link

decline commented Dec 16, 2022

  • What is the impact of this warning message for you? Do you just want to get rid of it (fully understandable) or is there more serious impact in terms of e.g. performance?

The issue is, that we still need ngcc to make it compatibly with Ivy. In our company's app, @sentry/angular is the last dependency that is still on View Engine, so we cannot get rid of ngcc for now. And that has some downsides for us, because for example we want to switch from npm to pnpm for performance reasons. But the need for ngcc is hindering us from doing this, because it introduces issues for running parallel builds when your workspace is build with pnpm.

So it's not just about the warning, but it has some deeper impacts on other things. I'm really looking forward to when @sentry/angular is finally on Ivy :)

@jdussouillez
Copy link

Any updates ? Do you have an ETA ?

I know you want to wait for v8 to drop support for Angular v11 and lower but it's only 25% completed. Meanwhile, Angular v15.2.0 will be released soon.

@alfaproject
Copy link

We are in the same boat as @decline. Also want to move away from yarn v1 but it's really difficult having to keep running ngcc to only compile @sentry/angular ):

@mredaelli
Copy link

Another reason we would like this sooner rather than later is that the esbuild builderfor Angular is now more capable and could be probably enough for our project, and way faster. But it only support Ivy

@Lonli-Lokli
Copy link

There is a PR for probably NG16, which will remove ngcc and make Ivy the only option
angular/angular-cli#24720

@Lms24
Copy link
Member

Lms24 commented Feb 16, 2023

Hey everyone, considering that v8 is gonna take a little longer to land, we decided last week to create a new Ivy-compatible Angular SDK package. Gonna start working on this soon. Stay tuned.

@yharaskrik
Copy link

Hey everyone, considering that v8 is gonna take a little longer to land, we decided last week to create a new Ivy-compatible Angular SDK package. Gonna start working on this soon. Stay tuned.

Yay!

@Lms24 Lms24 self-assigned this Feb 22, 2023
@Lms24
Copy link
Member

Lms24 commented Feb 24, 2023

Hey folks, #7264 is currently undergoing reviews and will soon be released as an experimental package. If you have thoughts on the package (e.g. build configuration), feel free to drop a note in the PR.

We're tracking everything that's related to creating and publishing this new package in #7265.

@Lms24
Copy link
Member

Lms24 commented Feb 27, 2023

We just released version 7.39.0 with @sentry/angular-ivy. I would really appreciate if folks following this thread would give it a try and report back if anything isn't working as expected. Please open a new issue, if you encounter a bug.

Migration from @sentry/angular to @sentry/angular-ivy:

  1. Remove @sentry/angular from your dependencies
  2. Add @sentry/angular-ivy as a dependency (available in version ^7.39.0)
  3. Replace all imports from @sentry/angular with @sentry/angular-ivy

Please make sure that all you @sentry/* packages are aligned to the same version (^7.39.0)

We're currently working on official docs but generally everything should work just like it did in @sentry/angular.
For anyone interested, this PR tracks the docs changes.

@Lms24 Lms24 closed this as completed Feb 27, 2023
@Lonli-Lokli
Copy link

Lonli-Lokli commented Feb 27, 2023

Hi @Lms24
I;ve updated to the latest version and switched to the ivy option, my build is failing

import { createErrorHandler, init as initSentry, routingInstrumentation, TraceService } from '@sentry/angular-ivy';
import { Integrations } from '@sentry/tracing';
...
if (environment.production) {
  enableProdMode();

  initSentry({
    dsn: 'UURL',
    integrations: [
      new Integrations.BrowserTracing({
        tracingOrigins: ['URL'],
        routingInstrumentation: routingInstrumentation
      })
    ],
    release: environment.build.version,
    tracesSampleRate: 0.25
  });
}

under strict: true

 Type 'import("D:/gitlab/cv-app/frontend/cv-ui/node_modules/@sentry/types/types/context").Contexts' is not assignable to type 'import("D:/gitlab/cv-app/frontend/cv-ui/node_modules/@sentry/hub/node_modules/@sentry/types/types/context").Contexts'.
                      'string' index signatures are incompatible.
                        Type 'Context | undefined' is not assignable to type 'Context'.
                          Type 'undefined' is not assignable to type 'Context'.

Error: node_modules/@sentry/hub/node_modules/@sentry/types/types/globals.d.ts:2:11 - error TS2451: Cannot redeclare block-scoped variable '__DEBUG_BUILD__'.

2     const __DEBUG_BUILD__: boolean;
            ~~~~~~~~~~~~~~~

  node_modules/@sentry/types/types/globals.d.ts:2:11
    2     const __DEBUG_BUILD__: boolean;
                ~~~~~~~~~~~~~~~
    '__DEBUG_BUILD__' was also declared here.


Error: node_modules/@sentry/types/types/globals.d.ts:2:11 - error TS2451: Cannot redeclare block-scoped variable '__DEBUG_BUILD__'.

2     const __DEBUG_BUILD__: boolean;
            ~~~~~~~~~~~~~~~

  node_modules/@sentry/hub/node_modules/@sentry/types/types/globals.d.ts:2:11
    2     const __DEBUG_BUILD__: boolean;
                ~~~~~~~~~~~~~~~
    '__DEBUG_BUILD__' was also declared here.

@Lonli-Lokli
Copy link

As an additional note, package.json's have different locked versions, is it expected?
angular-ivy

"dependencies": {
    "@sentry/browser": "7.39.0",
    "@sentry/types": "7.39.0",
    "@sentry/utils": "7.39.0",
    "tslib": "^2.3.0"
  },

browser

"dependencies": {
    "@sentry/core": "7.39.0",
    "@sentry/replay": "7.39.0",
    "@sentry/types": "7.39.0",
    "@sentry/utils": "7.39.0",
    "tslib": "^1.9.3"
  },

core

 "dependencies": {
    "@sentry/types": "7.39.0",
    "@sentry/utils": "7.39.0",
    "tslib": "^1.9.3"
  },

hub

"dependencies": {
    "@sentry/types": "7.3.0",
    "@sentry/utils": "7.3.0",
    "tslib": "^1.9.3"
  },

replay

"dependencies": {
    "@sentry/core": "7.39.0",
    "@sentry/types": "7.39.0",
    "@sentry/utils": "7.39.0"
  },

tracing

  "dependencies": {
    "@sentry/hub": "7.3.0",
    "@sentry/types": "7.3.0",
    "@sentry/utils": "7.3.0",
    "tslib": "^1.9.3"
  },

utils

 "dependencies": {
    "@sentry/types": "7.39.0",
    "tslib": "^1.9.3"
  },

@Lms24
Copy link
Member

Lms24 commented Feb 27, 2023

@Lonli-Lokli thanks for trying it out.

As an additional note, package.json's have different locked versions, is it expected

No, this is definitely not expected. All packages must be aligned on the same version. Seems like the @sentry/tracing package is still on 7.3.0 which causes these errors. Also, you should be good to delete the @sentry/hub package as we don't need it anymore (all its contents were moved to @sentry/core).

@Lonli-Lokli
Copy link

Lonli-Lokli commented Feb 27, 2023

@Lms24 Hold on, maybe it's my mistake :-) I see that not all refs has been updated

EDIT: Yes, it was my mistake, sorry about that. All refs except for tslib in angular-ivy are the same now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: angular Issues related to the Sentry Angular SDK Type: Improvement
Projects
None yet
Development

No branches or pull requests