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

fix(compiler-cli): Return original sourceFile instead of redirected sourceFile #26036

Closed
wants to merge 1 commit into from

Conversation

@JonWallsten
Copy link
Contributor

commented Sep 20, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

When using webpack dev server and AngularCompilerPlugin (maybe other combinations as well) all but the first compile always fails with the the following error:

Host should not return a redirect source file from `getSourceFile`

The problem is causes by duplicated definition files with different paths.
Example:

C:/Users/me/repo/web-app-angular/node_modules/@types/webpack/node_modules/source-map/source-map.d.ts
C:/Users/me/repo/web-app-angular/node_modules/@types/uglify-js/node_modules/source-map/source-map.d.ts

Typescript expects the redirect's target instead of the redirects source.
This fix solves that.
Issue Number: #22524

What is the new behavior?

The error message is not thrown. Fix is borrowed from here: ng-packagr/ng-packagr@c1fced0

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

This is definitely a hotfix. I don't have enough knowledge about TypeScript to solve the root cause. But this has been stopping our project since March, and based ont he up votes on the issue I'm not the only one.

@googlebot googlebot added the cla: yes label Sep 20, 2018

@JonWallsten JonWallsten force-pushed the JonWallsten:patch-issue-22524 branch 4 times, most recently from 42f8e6e to 800b878 Sep 20, 2018

@JonWallsten JonWallsten changed the title Patch issue 22524 fix(compiler-cli): Temp fix in program.ts issue Sep 20, 2018

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

Build seems to be failing because:

===== archive - skip deploy - commit not HEAD
fatal: unable to access 'https://github.com/angular/angular.git/': gnutls_handshake() failed: A TLS packet with unexpected length was received.
Fail

@JonWallsten JonWallsten force-pushed the JonWallsten:patch-issue-22524 branch from 800b878 to ded09d3 Sep 21, 2018

@kara kara added the comp: core label Sep 24, 2018

@yannickglt

This comment has been minimized.

Copy link

commented Sep 26, 2018

I confirm that it fixes watch on server. Is there a chance it get merged and released on an angular patch soon?

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

Is this just going to be ignored forever?

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@Splaktar

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

@JonWallsten sorry for the delay on this. It should start moving forward now.

Would it be possible to post a PR with the Ivy fix as well?

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@Splaktar Sure! I'll get one done by this weekend.

@JonWallsten JonWallsten force-pushed the JonWallsten:patch-issue-22524 branch from ded09d3 to 8b972a8 Jul 6, 2019

@JonWallsten JonWallsten requested a review from angular/fw-compiler as a code owner Jul 6, 2019

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

@Splaktar: I've updated the PR now. However, I don't know if there are more places. These are the two I've come across in my current project while bulding with Webpack. I did dig around some in the project but couldn't find any obvious ones. But then again. It's not crystal clear code-wise where this is an issue. There are around 40 places where getSourceFiles is used, but not all of them uses the source files.

@JonWallsten JonWallsten force-pushed the JonWallsten:patch-issue-22524 branch from 8b972a8 to d111b23 Jul 6, 2019

@JoostK

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

@JonWallsten your block here:

https://github.com/angular/angular/pull/26036/files#diff-a2da28fb9243bf20e75c1a423e6f8926R41

is acting only on source files from the delegate, which as the comment describes is a situation that should not occur as all source files are expected to be present in this.sfMap. It is exactly this map that contains the files from the old program, so those are the ones that we need to inspect.

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@JoostK: I totally missed the sf === undefined part. Fixed now!

@JonWallsten JonWallsten force-pushed the JonWallsten:patch-issue-22524 branch from ec43a42 to 08df399 Jul 9, 2019

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@JoostK: Is it possible to trigger tests again without a new commit?

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

@JonWallsten - hi Jon. Members of the angular org can retrigger CircleCI tests (possibly the PR author themselves can) by going to the CircleCI UI. CodeFresh doesn't have this ability (AFAIK) so that can only be re-run by a new commit.
A simple way to push a new run is git commit --amend --reuse-message=HEAD && git push --force-with-lease. Hope I am not teaching you something you already know there!

@JonWallsten JonWallsten force-pushed the JonWallsten:patch-issue-22524 branch from 08df399 to 70f37a8 Jul 9, 2019

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@petebacondarwin: That's a new one! Will be usefull! Thanks!

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@JoostK @petebacondarwin - the build seems to fail. And I have no idea what the error message means.

@JoostK

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

@JonWallsten neither do I, it seems like a flake. I restarted the workflow for you, lets hope that resolves the issue.

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

LOL! So did I. I'll leave @JoostK to help out here. I think it was probably a flake.

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@JoostK @petebacondarwin: Weird! Happends twice in a row. That's why I thought it was a real error. But third times the charm it seems! Thanks guys!

@JonWallsten JonWallsten force-pushed the JonWallsten:patch-issue-22524 branch from 70f37a8 to cc33dc0 Jul 9, 2019

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@JoostK: Let me know if there's anything more you'd need me to do!

// location.
// We return 'redirectInfo.unredirected' to return the original file and not the, by
// TypeScript, suggested target file.
if ((summary.sourceFile as any).redirectInfo) {

This comment has been minimized.

Copy link
@JoostK

JoostK Jul 9, 2019

Member

I would prefer the code to read as follows:

sf = summary.sourceFile;
const redirectInfo = (sf as any).redirectInfo;
if (redirectInfo !== undefined) {
  sf = redirectInfo.unredirected;
}
@@ -39,6 +39,12 @@ export class TypeCheckProgramHost implements ts.CompilerHost {
sf = this.delegate.getSourceFile(
fileName, languageVersion, onError, shouldCreateNewSourceFile);
sf && this.sfMap.set(fileName, sf);
} else if ((sf as any).redirectInfo) {

This comment has been minimized.

Copy link
@JoostK

JoostK Jul 9, 2019

Member

Also here, I think the following reads better:

} else {
  const redirectInfo = (sf as any).redirectInfo;
  if (redirectInfo !== undefined) {
    sf = redirectInfo.unredirected;
  }
}

Maybe the comment should mention that offering the redirect source file is not allowed by TypeScript, hence the effort to obtain the original source file.

@JonWallsten JonWallsten force-pushed the JonWallsten:patch-issue-22524 branch 2 times, most recently from 64361a2 to 5bd001a Jul 9, 2019

@JonWallsten JonWallsten force-pushed the JonWallsten:patch-issue-22524 branch from 5bd001a to 9bd0e96 Jul 9, 2019

@JonWallsten

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@JoostK: Done! Probably what have taken you much less time to do this yourself. I feel like a middleman! ;)
But I appreciate your patience.

@JoostK

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

@JonWallsten thank you for your perseverance! It is much appreciated 👍The compiler master @alxhub is not available this week, but I am pretty confident we can get this approved and merged soon.

@Splaktar

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

@JonWallsten many thanks for your contribution here!

@alxhub

alxhub approved these changes Jul 15, 2019

Copy link
Contributor

left a comment

👍

@JoostK

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

merge-assistance: please run presubmit

@matsko matsko closed this in 3166cff Jul 15, 2019

matsko added a commit that referenced this pull request Jul 15, 2019

@JonWallsten JonWallsten deleted the JonWallsten:patch-issue-22524 branch Jul 18, 2019

@thefliik

This comment has been minimized.

Copy link

commented Jul 21, 2019

That moment when you first encounter a really annoying Angular bug, only to realize that other people have been dealing with it for over a year and just fixed it for you.

Thank you @JonWallsten!!!!!! 🎉 🎉 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.