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.

Sign up for GitHub

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

Jump to bottom

amp-bind: Add function to print bind's scope #8876

Merged
merged 7 commits into from Apr 27, 2017
Merged

amp-bind: Add function to print bind's scope #8876

merged 7 commits into from Apr 27, 2017

Conversation

kmh287
Copy link
Contributor

@kmh287 kmh287 commented Apr 21, 2017 •
edited

Adds a function enabled in dev mode to print out bind's scope.

The function allows for scope to have circular references, though currently I'm not sure that it's possible for such references to get into bind's scope.

Fixes #8719

/to @choumx

@kmh287 kmh287 mentioned this pull request Apr 24, 2017
Closed
dreamofabear
dreamofabear reviewed Apr 25, 2017
View reviewed changes
Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Just a minor suggestion.

extensions/amp-bind/0.1/bind-impl.js Outdated
const index = seen.indexOf(value);
if (index !== -1) {
const name = seenNames[index];
return `**Circular reference to '${name}'**`;
Copy link

Choose a reason for hiding this comment

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

Let's use node's handling of circular references:

> var a = {};
undefined
> a.a = a;
{ a: [Circular] }

You can remove seenNames.

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

dreamofabear
dreamofabear approved these changes Apr 26, 2017
View reviewed changes
Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Please also add this to the public docs.

extensions/amp-bind/0.1/bind-impl.js Outdated
@@ -154,6 +154,7 @@ export class Bind {
// Expose for testing on dev.
if (getMode().localDev) {
AMP.reinitializeBind = this.initialize_.bind(this);
AMP.printAmpState = this.printAmpState_.bind(this);
Copy link

Choose a reason for hiding this comment

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

Nit: AMP.printState

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

kmh287
kmh287 commented Apr 26, 2017
View reviewed changes
Copy link
Contributor Author

@kmh287 kmh287 left a comment

Choose a reason for hiding this comment

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

Added amp-bind.md to the PR.

extensions/amp-bind/0.1/bind-impl.js Outdated
@@ -154,6 +154,7 @@ export class Bind {
// Expose for testing on dev.
if (getMode().localDev) {
AMP.reinitializeBind = this.initialize_.bind(this);
AMP.printAmpState = this.printAmpState_.bind(this);
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

@kmh287 kmh287 merged commit 6509720 into ampproject:master Apr 27, 2017
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
@kmh287 @mrjoro
amp-bind: Add function to print bind's scope (ampproject#8876)
c403778
KenneyE pushed a commit to spotxchange/amphtml that referenced this pull request May 3, 2017
@kmh287
amp-bind: Add function to print bind's scope (ampproject#8876)
3b4e657
@zhangsu
Copy link
Member

zhangsu commented May 20, 2017 •
edited

Sorry for chipping in here, but is this really in prod? I'm in development mode (#development=1 in the fragment) and using

<script async custom-element="amp-bind" src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>

But in console, I get this:

AMP.printState()
VM1646:1 Uncaught TypeError: AMP.printState is not a function
    at <anonymous>:1:5
(anonymous) @ VM1646:1
AMP.printState
undefined
AMP
Object {win: Window, config: Object, BaseElement: function, BaseTemplate: function, registerElement: function…}

The AMP namespace is there, but it doesn't have a printState attribute.

@kmh287
Copy link
Contributor Author

kmh287 commented May 24, 2017

@zhangsu #9460

@zhangsu
Copy link
Member

zhangsu commented May 24, 2017

Great, thanks! I didn't realize it was only enabled for localhost...

@oimos
Copy link

oimos commented Apr 1, 2018

I get "undefined" when I do AMP.printState() on console although the page is on localhost.
Do you know how to resolve this? Do I need to turn on some experimental modes for this?

@dreamofabear
Copy link

@stoneshower Looks like you still need #development=1 or #log=4. We should change this to use console.info instead.

@oimos
Copy link

oimos commented Apr 3, 2018 •
edited

@choumx with #development=1 and #log=4, it shows the output. Thanks!

@AcidRaZor
Copy link

where is #log=4 documented? I also can't find any reference to AMP.printState on any of your documentation either. https://ampbyexample.com/components/amp-bind/

you would think if you want your technology to be adopted with the looming "google will index you higher than your competitor" axe hanging above one's head, that basic debugging tools would help.

@dreamofabear
Copy link

@AcidRaZor Thanks for the feedback. AMP.printState no longer requires #development=1 and is documented in the component docs here.

We're also working on more powerful debugging tools, e.g. #16830. I'd love to eventually have a Chrome extension similar to React Developer Tools, but unfortunately haven't had the time to implement this yet. Any interest in contributing to AMP? 😋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamofabear dreamofabear dreamofabear approved these changes

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@kmh287 @zhangsu @oimos @dreamofabear @AcidRaZor @ampprojectbot