Skip to content

Conversation

@caitp
Copy link
Contributor

@caitp caitp commented Jun 12, 2015

Plugins may opt to return a DiffResult themselves, and avoid the
need to calculate a diff

@caitp
Copy link
Contributor Author

caitp commented Jun 12, 2015

@IgorMinar I've experimented with converting plugins to return DiffResults, but have had (once again) issues with dead symlinks, relating to either MultiCopy or some of the Stew plugins. So it might need some work.

The performance for subsequent builds does seem to be very good when the build works correctly though (with just a handful of plugins updated) ~on the order of 25ms per tree

@caitp caitp added feature Issue that requests a new feature area: build & ci Related the build and CI infrastructure of the project action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 12, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

returning a promise for a diffresult should be possible too. no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, and there's code handling that case here --- but it seems weird to have Promise<any> | Promise<DiffResult>, since any is kind of catch-all

Copy link
Contributor

Choose a reason for hiding this comment

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

how about Promise<undefined|DiffResult>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@IgorMinar IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 19, 2015
@IgorMinar IgorMinar assigned caitp and unassigned IgorMinar Jun 19, 2015
@IgorMinar IgorMinar added this to the alpha-29 milestone Jun 19, 2015
@caitp
Copy link
Contributor Author

caitp commented Jun 19, 2015

I think that addresses each comment

Plugins may opt to return a DiffResult themselves, and avoid the need to calculate a diff
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment that this.treeDiffer attempts to reads the diff from the input tree where we stash after the rebuild of that tree is done.

@IgorMinar
Copy link
Contributor

otherwise this looks good

@IgorMinar IgorMinar added the action: merge The PR is ready for merge by the caretaker label Jun 19, 2015
@caitp caitp closed this in d575915 Jun 20, 2015
@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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes feature Issue that requests a new feature

3 participants