Skip to content

build(brocolli): convert brocolli-ts2dart to use TreeDiffer#1733

Merged
caitp merged 1 commit intoangular:masterfrom
caitp:issue-1720
May 14, 2015
Merged

build(brocolli): convert brocolli-ts2dart to use TreeDiffer#1733
caitp merged 1 commit intoangular:masterfrom
caitp:issue-1720

Conversation

@caitp
Copy link
Copy Markdown
Contributor

@caitp caitp commented May 7, 2015

Blocked on #1719

Closes #1720

@caitp caitp force-pushed the issue-1720 branch 2 times, most recently from 5945451 to 11563b0 Compare May 7, 2015 18:42
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.

is this just temporary? shouldn't this come in via options?

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.

they probably should come via options

@IgorMinar
Copy link
Copy Markdown
Contributor

the rest looks good to me

@caitp
Copy link
Copy Markdown
Contributor Author

caitp commented May 8, 2015

PTAL --- added code to remove unneeded files from cache

@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented May 8, 2015

Travis is not happy, can we get it green?

@IgorMinar
Copy link
Copy Markdown
Contributor

looks like a clang-format issue. @caitp can you fix please?

also what's up with the dgeni warnings? that should not be related to this change.

@caitp
Copy link
Copy Markdown
Contributor Author

caitp commented May 8, 2015

do you mean the dgeni issues with @link notations? I think pete is working on that, it's been broken for a while

@IgorMinar
Copy link
Copy Markdown
Contributor

yeah. those warnings.

On Fri, May 8, 2015 at 11:25 AM ⭐caitp⭐ [email protected] wrote:

do you mean the dgeni issues with @link notations? I think pete is
working on that, it's been broken for a while


Reply to this email directly or view it on GitHub
#1733 (comment).

@caitp
Copy link
Copy Markdown
Contributor Author

caitp commented May 8, 2015

but yes, those aren't related to this change, it's related to migrating to TypeScript, afaik

@caitp
Copy link
Copy Markdown
Contributor Author

caitp commented May 8, 2015

so, i take it it looks alright?

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.

this name is going to show up in logs, can you rename it to the shorter TS2DartTranspiler please?

@IgorMinar
Copy link
Copy Markdown
Contributor

the rest looks fine. can you double check what is being emitted and remove only the files emitted by the transpiler.

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.

I don't think that we can do it this way. we need to check if the input contains the dart file just like we do on lines 34-35.

it's possible that the input was:

- fileA.ts
- fileA.dart

then the transpiler will not emit anything and we will reuse the fileA.dart file as the output.

if later fileA.ts is removed, we should not remove fileA.dart because we didn't emit it. it is the responsibility of the previous plugin that gave us a tree that contained fileA.dart file to remove it.

so we need to reimplement the input tree checking logic here as well.

@caitp
Copy link
Copy Markdown
Contributor Author

caitp commented May 9, 2015

find tmp/diffing_plugin_wrapper-cache_path-JGpT3tqi.tmp/ -type f | sed -En 's|.*/[^/]+\.([^/.]+)$|\1|p' | sort -u
dart

So, just dart files I guess

@caitp caitp force-pushed the issue-1720 branch 2 times, most recently from 8c40959 to 01d18a8 Compare May 9, 2015 01:33
@caitp caitp force-pushed the issue-1720 branch 2 times, most recently from e7b25c8 to 3dcdd7e Compare May 12, 2015 23:01
@IgorMinar
Copy link
Copy Markdown
Contributor

is this ready to go? I see that some tests failed on travis. I wonder if that's a flake. I restarted the build to see...

@IgorMinar
Copy link
Copy Markdown
Contributor

ci is green..

@caitp
Copy link
Copy Markdown
Contributor Author

caitp commented May 14, 2015

Should be ok to land then.

@caitp caitp merged commit 3969009 into angular:master May 14, 2015
@caitp caitp removed the in progress label May 14, 2015
@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 6, 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.

Make ts2dart broccoli plugin incremental

4 participants