Skip to content

Proxying the form to call the prototype methods/properties#6720

Merged
dvoytenko merged 6 commits intoampproject:masterfrom
dvoytenko:proxy2
Dec 22, 2016
Merged

Proxying the form to call the prototype methods/properties#6720
dvoytenko merged 6 commits intoampproject:masterfrom
dvoytenko:proxy2

Conversation

@dvoytenko
Copy link
Copy Markdown
Contributor

To be discussed: any better way of doing this?

}
});

form['$p'] = proxy;
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.

@dvoytenko just double checking, should be $p and not $proxy?

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.

Yes. Was thinking about changing it to $p since it's a lot shorter.



/**
* @param {!HTMLFormElement} form
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.

This docs for this should be the URL to the Medium post about it :)

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.

Until then we will need something here.

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

* @return {!Object}
*/
export function installFormProxy(form) {
const proxy = {};
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.

Can we have a proxy prototype (that we initialize on the first form?

The proxy instance would then just have a private field like instance_ and the getters would access the form through this.instance_

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

*/
function FormProxy(form) {
/** @private @const {!HTMLFormElement} */
this.form = form;
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.

trailing _

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

Copy link
Copy Markdown
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

LG, but add extensive docs.

const properties = win.Object.getOwnPropertyDescriptors(prototype);
for (const name in properties) {
if (win.Object.hasOwnProperty(FormProxyProto, name) ||
name == 'constructor') {
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 is redundant, it'll never be true without the proceeding conditional being true.

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.

Removed

@dvoytenko dvoytenko changed the title TBD: proxying the form to call the prototype methods/properties Proxying the form to call the prototype methods/properties Dec 21, 2016
for (const name in properties) {
if (win.Object.hasOwnProperty(FormProxyProto, name) ||
name == 'constructor') {
if (win.Object.hasOwnProperty(FormProxyProto, name)) {
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.

Oh, and this should be .call, and really Object.prototype.hasOwnProperty.call. Or we created a helper.

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.

@dvoytenko dvoytenko merged commit 9e181b4 into ampproject:master Dec 22, 2016
@dvoytenko dvoytenko deleted the proxy2 branch December 22, 2016 19:01
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Jan 3, 2017
…t#6720)

* TBD: proxying the form to call the prototype methods/properties

* tests

* FormProxy.prototype

* docs

* use Object.prototype.hasOwnProperty

* fixes
jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017
…t#6720)

* TBD: proxying the form to call the prototype methods/properties

* tests

* FormProxy.prototype

* docs

* use Object.prototype.hasOwnProperty

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants