Skip to content

feat(@angular-devkit/build-angular): enable sourcemaps by default when using karma#12291

Merged
hansl merged 1 commit intoangular:masterfrom
alan-agius4:sourcemaps_on_default
Sep 18, 2018
Merged

feat(@angular-devkit/build-angular): enable sourcemaps by default when using karma#12291
hansl merged 1 commit intoangular:masterfrom
alan-agius4:sourcemaps_on_default

Conversation

@alan-agius4
Copy link
Copy Markdown
Collaborator

@alan-agius4 alan-agius4 commented Sep 18, 2018 •
edited
Loading

I tried to do a regression tests like @filipesilva suggested

  it('sourcemaps are enabled by default', (done) => {
    host.writeMultipleFiles({
      'src/app/app.component.spec.ts': 'console.log(new Error())',
    });

    const logger = new TestLogger('karma-sourcemaps');

    runTargetSpec(host, karmaTargetSpec, undefined, DefaultTimeout, logger).pipe(
      tap((buildEvent) => expect(buildEvent.success).toBe(false)),
      tap(() => {
        console.log(logger);
        expect(logger.includes('/app/app.component.spec.ts')).toBe(true)
      }),
    ).toPromise().then(done, done.fail);
  });

However the problem is that the stacktrace always contains ts paths. The only difference when enabling sourcemaps is the contents of the files, as shown below;

Sourcemaps enabled
image

No sourcemaps
image

Closes #12282 and Potentially fixes #11672

@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Sep 18, 2018
@alan-agius4 alan-agius4 mentioned this pull request Sep 18, 2018
Closed
@alan-agius4
Copy link
Copy Markdown
Collaborator Author

alan-agius4 commented Sep 18, 2018

Seems like this solved the code coverage branch detection issue as well.

screen shot 2018-09-18 at 18 30 07

@filipesilva
Copy link
Copy Markdown
Contributor

filipesilva commented Sep 18, 2018

Yeah code coverage will never work without sourcemaps, so that makes sense.

@hansl hansl merged commit fce686d into angular:master Sep 18, 2018
@alan-agius4 alan-agius4 deleted the sourcemaps_on_default branch September 18, 2018 19:40
@benlesh benlesh mentioned this pull request Sep 19, 2018
Closed
@corcorb corcorb mentioned this pull request Sep 21, 2018
Closed
@Teamop
Copy link
Copy Markdown
Contributor

Teamop commented Oct 12, 2018

@filipesilva could this PR also merged to v6.x? seems right now it's just in the v7.

@filipesilva
Copy link
Copy Markdown
Contributor

filipesilva commented Oct 12, 2018

@Teamop unfortunately no, since it is a feature. Features only go on minor releases, and we don't plan on making another minor for 6.x. You can set the "sourceMap": true property on your test configuration and it will work the same though.

JLHwung pushed a commit to easyops-cn/ng-zorro-antd that referenced this pull request Nov 18, 2018
chore: enable sourceMaps when using karma
f6891ea 
See angular/angular-cli#12291 (comment)
@JLHwung JLHwung mentioned this pull request Nov 20, 2018
Closed
3 tasks
@alan-agius4 alan-agius4 mentioned this pull request Nov 29, 2018
Closed
@TeoTN
Copy link
Copy Markdown

TeoTN commented Dec 4, 2018 •
edited
Loading

It was working fine with v~0.6.8 and stopped working with v6-lts and you call it a feature? I hit that error because I updated the lib with npm audit, I'd vastly appreciate if you could name it fix and add to Long Term Support release...

@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Sep 12, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@filipesilva filipesilva filipesilva approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

target: major This PR is targeted for the next major release

Projects

None yet

Milestone

No milestone

6 participants

@alan-agius4 @filipesilva @Teamop @TeoTN @hansl @googlebot