Skip to content

Ivy guide update - #29506

Closed
filipesilva wants to merge 2 commits into
angular:masterfrom
filipesilva:ivy-guide-update
Closed

filipesilva wants to merge 2 commits into
angular:masterfrom
filipesilva:ivy-guide-update

Conversation

@filipesilva

@filipesilva filipesilva commented Mar 25, 2019 •

Copy link
Copy Markdown
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • 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 new behavior?

Ivy guide is updated with up to date steps.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Blocked on angular/angular-cli#13948 and angular/angular-cli#13968

@mary-poppins

Copy link
Copy Markdown

You can preview 8ced065 at https://pr29506-8ced065.ngbuilds.io/.

@ngbot ngbot Bot added this to the needsTriage milestone Mar 25, 2019
Comment thread aio/content/guide/ivy.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

for version 8, this tsconfig is not under src anymore. it's in the root.

tsconfig.app.json

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread aio/content/guide/ivy.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

src/tsconfig.app.json -> tsconfig.app.json

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@jbogarthyde jbogarthyde mentioned this pull request Mar 26, 2019
4 of 14 tasks
Comment thread aio/content/guide/ivy.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

say where are the default build options? (which file)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread aio/content/guide/ivy.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't you still need to restore node_modules/ to clean up typings modified by ngcc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

AFAIK it's ok to leave it there for VE compilations because the typings were additive. So if you want to switch back and forth there is no need to reinstall. Also if you want to switch, and you reinstall, the first build will again take longer.

Is there any real risk that new typings will be used by accident?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, even if additive, the typings will be wrong. (For example, certain classes will claim thay have some properties that they don't.)
I don't know of any reason for people to want to use or care about those properties, so they are probably harmless, but I might miss something.

@mary-poppins

Copy link
Copy Markdown

You can preview ac5bc0f at https://pr29506-ac5bc0f.ngbuilds.io/.

@filipesilva
filipesilva marked this pull request as ready for review April 1, 2019 13:23
@filipesilva
filipesilva requested a review from a team April 1, 2019 13:23
@filipesilva filipesilva added target: major This PR is targeted for the next major release action: merge The PR is ready for merge by the caretaker labels Apr 1, 2019
@jasonaden jasonaden closed this in ee7c5ed Apr 1, 2019
jasonaden pushed a commit that referenced this pull request Apr 1, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@angular-automatic-lock-bot

Copy link
Copy Markdown

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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants