Skip to content

refactor(ivy): Move instructions back to ɵɵ - #30546

Closed
benlesh wants to merge 2 commits into
angular:masterfrom
benlesh:FW-1337_frog_eyes_forever
Closed

benlesh wants to merge 2 commits into
angular:masterfrom
benlesh:FW-1337_frog_eyes_forever

Conversation

@benlesh

@benlesh benlesh commented May 17, 2019

Copy link
Copy Markdown
Contributor

There is an encoding issue with using delta Δ, where the browser will attempt to detect the file encoding if the character set is not explicitly declared on a <script/> tag, and Chrome will find the Δ character and decide it is window-1252 encoding, which misinterprets the Δ character to be some other character that is not a valid JS identifier character

So back to the frog eyes we go.

    __
   /ɵɵ\
  ( -- ) - I am ineffable. I am forever.
 _/    \_
/  \  /  \
==  ==  ==

@benlesh benlesh added refactoring Issue that involves refactoring or code-cleanup target: major This PR is targeted for the next major release comp: ivy risk: medium labels May 17, 2019
@benlesh
benlesh requested a review from IgorMinar as a code owner May 17, 2019 22:46
@benlesh
benlesh requested review from a team May 17, 2019 22:46
@ngbot ngbot Bot modified the milestone: needsTriage May 17, 2019
@benlesh
benlesh force-pushed the FW-1337_frog_eyes_forever branch from 5514aaf to 4fa0e9d Compare May 17, 2019 23:36

@kara kara left a comment •

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.

Mostly looks good. Main issue is the public API guard looks off to me.

Also can we have a descriptive commit that doesn't say "frog eyes"? e.g.

refactor(ivy): switch back to ɵɵ prefix for instructions

Comment thread packages/compiler-cli/test/compliance/mock_compile.ts Outdated
Comment thread tools/public_api_guard/core/core.d.ts Outdated
@kara kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels May 18, 2019
@benlesh
benlesh force-pushed the FW-1337_frog_eyes_forever branch 2 times, most recently from 1edb62e to e67e01a Compare May 18, 2019 01:50
@benlesh benlesh changed the title refactor(ivy): Back to frog eyes ɵɵ refactor(ivy): Move instructions back to ɵɵ May 18, 2019
@benlesh
benlesh requested a review from kara May 18, 2019 02:08
@benlesh

benlesh commented May 20, 2019

Copy link
Copy Markdown
Contributor Author

I'm really not sure why the bundle size went up so much. It shouldn't have changed much if at all. 🤔

@ocombe

ocombe commented May 20, 2019

Copy link
Copy Markdown
Contributor

@benlesh it was an issue with the data in the database, restart circleCI and it should be fixed

Comment thread tools/ts-api-guardian/index.bzl 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.

Faces everywhere!

benlesh added 2 commits May 20, 2019 10:48
There is an encoding issue with using delta `Δ`, where the browser will attempt to detect the file encoding if the character set is not explicitly declared on a `<script/>` tag, and Chrome will find the `Δ` character and decide it is window-1252 encoding, which misinterprets the `Δ` character to be some other character that is not a valid JS identifier character

So back to the frog eyes we go.

```
    __
   /ɵɵ\
  ( -- ) - I am ineffable. I am forever.
 _/    \_
/  \  /  \
==  ==  ==
```
@benlesh
benlesh force-pushed the FW-1337_frog_eyes_forever branch from d86cdba to d68cb81 Compare May 20, 2019 17:48
@benlesh

benlesh commented May 20, 2019

Copy link
Copy Markdown
Contributor Author

presubmit

@kara kara left a comment

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.

LGTM

@kara kara added action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 20, 2019
@benlesh benlesh added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit labels May 20, 2019
@ngbot ngbot Bot added the action: merge The PR is ready for merge by the caretaker label May 20, 2019
@jasonaden jasonaden closed this in d7eaae6 May 20, 2019
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
There is an encoding issue with using delta `Δ`, where the browser will attempt to detect the file encoding if the character set is not explicitly declared on a `<script/>` tag, and Chrome will find the `Δ` character and decide it is window-1252 encoding, which misinterprets the `Δ` character to be some other character that is not a valid JS identifier character

So back to the frog eyes we go.

```
    __
   /ɵɵ\
  ( -- ) - I am ineffable. I am forever.
 _/    \_
/  \  /  \
==  ==  ==
```

PR Close angular#30546
@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 15, 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 merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note refactoring Issue that involves refactoring or code-cleanup risk: medium target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants