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(@angular-devkit/build-angular): add missing declaration types … #13421

Merged
merged 1 commit into from
Jan 14, 2019

Conversation

gatthias
Copy link
Contributor

…in build_angular schemas.

Fixes issue #13388

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@gatthias
Copy link
Contributor Author

Hi,

I made this pull request to specifically address issue #13388.
I get that this issue has been closed by @hansl as "working as intended", but I kindly disagree with this as this breaks the ability to cleanly compile custom builders deriving or using the BrowserBuilder. I even wonder how transpilation worked on your side since 7.2.0 given these broken types declarations links.

This PR fixes this, and allow fellow devs to continue working with their own builders setup while waiting for v8.

To break circular dependencies, I had to move two declarations NormalizedSourceMaps and NormalizedOptimization away from utils. I chose to declare them in browser/schema as imports from this file were already present for every other files that needed these declarations. This is not ideal, but I thought that putting these right between NormalizedBrowserBuilderSchema and it's above TODO comment would be the best compromise. Please, do tell if you disagree.

Sincerely sorry for the little mess with commits:

  • First one didn't have the right author information and did not pass linting
  • Second one corrected lint issues, I marked it "refactor" rather than "style" as some code needed to be moved to avoid circular dependencies. Hope I did well there.
  • Subsequent ones were needed to fix the first commit's authorship issue.

Finally, I'm not sure why two items of test-large failed on CI when it worked locally:

20 Browser Builder rebuilds

    √ rebuilds on TS file changes (10 secs)

    √ rebuilds on CSS changes (8 secs)

    √ type checks on rebuilds (12 secs)

    √ rebuilds on type changes (9 secs)

Thank you for your amazing work, I hope this edit can help.

Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Thanks for this, kindly squash all commits.

The failing test seems to be a flake.

@googlebot
Copy link

CLAs look good, thanks!

@gatthias
Copy link
Contributor Author

gatthias commented Jan 13, 2019

Well I just learned about squashing. I hope it's all good, Bot seems okay with this.
Thanks, learning everyday 👍 !

EDIT: I'll be damned, these actions broke linting again. Back to it...

@gatthias
Copy link
Contributor Author

gatthias commented Jan 13, 2019

Okay seems all good to me. I believe the last merge status check is on your side.
I'll get back to use these changes now !

One last thing though, as this PR moves two declarations, do you think we should export back these two types from utils for backward compatibility ? Something like :

utils/index.ts
export { NormalizedOptimization, NormalizedSourceMaps } from '../browser/schema';

Kinda ugly but it might prevent further issues ?

EDIT: Just checked, it wouldn't break linting.

Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM

@alan-agius4
Copy link
Collaborator

@opulisDynamics I don't see the need for that.

@alan-agius4 alan-agius4 added the target: patch This PR is targeted for the next patch release label Jan 14, 2019
@kyliau kyliau merged commit 59d9735 into angular:master Jan 14, 2019
@imzachwolf
Copy link

My build was fixed with "@angular-devkit/build-angular": "0.13.9". Thanks folks.

@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants