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): report not existing files as errors #9690

Merged
merged 1 commit into from Jun 29, 2016

Conversation

tbosch
Copy link
Contributor

@tbosch tbosch commented Jun 29, 2016

No description provided.

}
return Promise.resolve(compilerHost.readFile(s));
}
};
Copy link
Member

Choose a reason for hiding this comment

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

No test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have a unit test setup for codegen yet. Especially for errors. That would require some more thinking that I can't do right now...

@jelbourn
Copy link
Member

LGTM, I suppose, but maybe add a TODO for testing this behavior.

@naomiblack naomiblack added this to the 2.0.0-rc.4 milestone Jun 29, 2016
const xhr: compiler.XHR = {
get: (s: string) => {
if (!compilerHost.fileExists(s)) {
throw new Error(`File not found: ${s}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make the error even less ambiguous by prefixing it with "Compilation failed. Resource file not found: ..."

@IgorMinar
Copy link
Contributor

lgtm otherwise

@IgorMinar IgorMinar added pr_state: LGTM action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 29, 2016
@tbosch
Copy link
Contributor Author

tbosch commented Jun 29, 2016

Thanks. Changed the error message and added a TODO for adding tests.

@tbosch tbosch removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 29, 2016
get: (s: string) => {
if (!compilerHost.fileExists(s)) {
// TODO: We should really have a test for error cases like this!
throw new Error(`Compilation failed. Resource file not found: ${s}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is re-caught at https://github.com/angular/angular/blob/master/modules/%40angular/compiler-cli/src/main.ts#L31 with the "compilation failed" message so you maybe only need the "Resource file not found" in the message here? Check what the UI looks like for failures...

@tbosch tbosch merged commit e81dea6 into angular:master Jun 29, 2016
@tbosch tbosch deleted the fix_compile_err branch June 29, 2016 16:41
@angular-automatic-lock-bot
Copy link

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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants