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.
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
amp-bind: Expose encodeURI and encodeURIComponent in bind #8723
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update amp-bind.md.
@@ -268,6 +268,11 @@ describe('BindExpression', () => { | |||
}).to.throw(unsupportedFunctionError); | |||
}); | |||
|
|||
it('should support encodeURI and encodeURIComponent', () => { | |||
expect(evaluate('encodeURI("Hello World")')).to.equal('Hello%20World'); | |||
expect(evaluate('encodeURIComponent("#foo=bar")')).to.equal('%23foo%3Dbar'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a fragment or URL params? We should test the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍
@@ -268,6 +268,11 @@ describe('BindExpression', () => { | |||
}).to.throw(unsupportedFunctionError); | |||
}); | |||
|
|||
it('should support encodeURI and encodeURIComponent', () => { | |||
expect(evaluate('encodeURI("Hello World")')).to.equal('Hello%20World'); | |||
expect(evaluate('encodeURIComponent("#foo=bar")')).to.equal('%23foo%3Dbar'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
expect(evaluate('encodeURI("http://www.google.com/s p a c e.html")')) | ||
.to.equal('http://www.google.com/s%20p%20a%20c%20e.html'); | ||
expect(evaluate('encodeURIComponent("hello world")')) | ||
.to.equal('hello%20world'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This test for encodeURIComponent
tests the same functionality as the one for encodeURI
. Recommend testing proper encoding for some of these chars: ; , / ? : @ & = + $ #
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that's necessary. We don't need to test the correctness of the functions, just that using them in bind expressions works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctness is currently an implementation detail. 😄 It is a nitpick though.
Adds support for
encodeURI
andencodeURIComponent
in bind.Partial for #8700
/to @choumx