Split url-replacement to var-substitution to allow non-url subs by mkhatib · Pull Request #6257 · ampproject/amphtml

mkhatib · 2016-11-20T02:57:14Z

As per this discussion moving var-sub to its own service.

jridgewell

LGTM, but I'm just assuming this is a copy-paste, right?

jridgewell · 2016-11-21T18:34:08Z

src/service/url-replacements-impl.js

+  installVarSubstitutionForEmbed(ampdoc, embedWin, varSource);
  installServiceInEmbedScope(embedWin, 'url-replace',
-      new UrlReplacements(ampdoc, varSource));
+      new UrlReplacements(ampdoc));


Do we need to install installVarSubstitutionServiceForDoc?

I am installing it for the embed scope, I think that should be enough no?

cc/ @avimehta

I believe it should be enough because service for doc is installed elsewhere. otherwise, will it be okay to call it here again? it should be a noop if the service is already installed.

What do you mean service for doc is installed elsewhere, you mean runtime.js call?

It would be a noop if we call it again here, but I don't understand why would we call it here since we haven't been installing it here before this change?

jridgewell · 2016-11-21T18:35:03Z

src/service/url-replacements-impl.js

+} from './var-substitution-impl';
+import {varSubstitutionForDoc} from '../var-substitution';
+import {
+    VariableSource,


What are these being imported for?

The gulp check-types throw errors because it doesn't know the Defs and the VariableSource type. I've noticed that we do add them to externs sometimes, should I do that instead?

This still breaks linting for example giving me VariableSource is defined but never used problem. So not sure how to best fix this.

They types should be something like {../path/to/variable-source.VariableSource}?

mkhatib · 2016-11-22T20:05:15Z

Yes, this is mostly cut-paste with minimal changes to url-replacement-impl, I kept the tests there on purpose but also copied them to also cover var-substitution-impl separately.

avimehta · 2016-11-22T20:17:10Z

/cc @dvoytenko Initially when I created VariableSource, Dima asked me to not make VariableSource as a service(it was a service initially). Just want to make sure he is aware of this change.

avimehta · 2016-11-22T20:21:33Z

Also, A4AVariableSource currently "gets" the global variable source from url-replacements service. We can probably change it do directly request the service instead if we decide to go ahead with this PR.

jridgewell · 2016-11-22T21:17:05Z

src/service/url-replacements-impl.js

-    VariableSource,
    ResolverReturnDef,
    SyncResolverDef,
 } from './variable-source';


Ditto for these imports.

I tried this, didn't work because maybe defs are not exported, will export and try again.

dvoytenko · 2016-11-23T00:17:18Z

If we have good use cases, I don't mind, but is there a real value to have it as a external service?

jridgewell · 2016-11-23T00:25:01Z

Both Analytics and Forms (is it the forms installed with runtime or the forms extension?) will depend on now. Code size win?

mkhatib · 2016-11-23T00:28:24Z

Forms extension. We also, wanted to make sure to separate general variable substitution from URL-specific replacements for specific extra protection for url-replacements (like the protocol mismatching). Forms will use var-subs inside hidden fields directly rather than the full URL.

dvoytenko · 2016-11-23T00:50:35Z

@jridgewell The url replacer is a core service so probably no code size effect.
@mkhatib That seems like a smallish nuance of API.

There's on issue with service proliferation: they are kind of hard to maintain when they need to be customized for embeds. Where we had a single service customized, now there are two that depend on each other in a bit confusing way.

mkhatib · 2016-11-23T00:55:21Z

Do we just not do this and expose a URL-replacement API that is expandStringSync and expandStringAsync instead? It just becomes a confusing API specially the naming, so maybe we should refactor url-replacement to be var-substitution and have APIs that are expandUrlSync and expandUrlAsync.

We really need a better way to handle embeds though. In my other PRs, it's confusing to know which context is this going to run in and how to handle the different contexts.

I don't have a preference of which path we take in this PR, happy to close it and send a new one with new APIs to url-replacement, I'll leave the say to @jridgewell

jridgewell · 2016-11-23T01:02:16Z

I'll leave the say to @jridgewell

Defering to @dvoytenko. 😛

If we go the other route, I'd prefer the URL replacements to be the specialized method names.

dvoytenko · 2016-11-23T01:03:53Z

I'd keep one service and come up with the good new method names and deprecate the old ones and cleanup in a couple of weeks.

mkhatib · 2016-11-23T01:09:28Z

So I am assuming naming is still going to be url-replacement for the service?

Suggestions for good API names?

Old expandSync -> deprecate (alias to expandUrlSync until removed)
Old expandAsync -> deprecate (alias to expandUrlAsync until removed)
expandUrlSync -> Does extra URL protocol checks
expandUrlAsync -> Does extra URL protocol checks
expandStringSync -> No URL protocols checks - but usage MUST be reviewed - enforced through presubmits.
expandStringAsync -> No URL protocols checks - but usage MUST be reviewed - enforced through presubmits.

dvoytenko · 2016-11-23T01:13:53Z

This sounds good. To confirm though, do forms need string interpolation or just variable resolution?

mkhatib · 2016-11-23T02:24:46Z

Sent a new PR #6323

mkhatib added the NEEDS REVIEW label Nov 20, 2016

mkhatib assigned jridgewell Nov 20, 2016

mkhatib mentioned this pull request Nov 20, 2016

Allow variable substitutions in hidden fields. #6130

Merged

Split url-replacement to var-substitution to allow non-url subs.

0ace16e

mkhatib force-pushed the var-subs branch from 088996e to 0ace16e Compare November 20, 2016 03:05

jridgewell approved these changes Nov 21, 2016

View reviewed changes

fix lint

c87ee70

jridgewell reviewed Nov 22, 2016

View reviewed changes

Fix def

38efe67

mkhatib closed this Nov 23, 2016

This was referenced Nov 23, 2016

Deprecate Usage of expandSync and expandAsync #6322

Closed

Introduce expandStringAsync and expandStringSync in url-replacement #6323

Merged

Conversation

mkhatib commented Nov 20, 2016

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhatib commented Nov 22, 2016

Uh oh!

avimehta commented Nov 22, 2016

Uh oh!

avimehta commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvoytenko commented Nov 23, 2016

Uh oh!

jridgewell commented Nov 23, 2016

Uh oh!

mkhatib commented Nov 23, 2016

Uh oh!

dvoytenko commented Nov 23, 2016

Uh oh!

mkhatib commented Nov 23, 2016

Uh oh!

jridgewell commented Nov 23, 2016

Uh oh!

dvoytenko commented Nov 23, 2016

Uh oh!

mkhatib commented Nov 23, 2016

Uh oh!

dvoytenko commented Nov 23, 2016

Uh oh!

mkhatib commented Nov 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants