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.

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

feat: $designer.id based URL #4242

Merged
merged 14 commits into from
Sep 30, 2020
Merged

feat: $designer.id based URL #4242

merged 14 commits into from
Sep 30, 2020

Conversation

yeze322
Copy link
Contributor

@yeze322 yeze322 commented Sep 23, 2020

Description

closes #4170

triggers[0].actions[1] -> triggers['475044'].actions['485963']

Task Item

Screenshots

designer-url

@coveralls
Copy link

coveralls commented Sep 23, 2020

Coverage Status

Coverage increased (+0.2%) to 55.591% when pulling e858318 on nav/designer-url into 1d272a8 on main.

@yeze322 yeze322 requested a review from lei9444 September 23, 2020 11:12
@yeze322 yeze322 changed the title $designer.id based URL feat: $designer.id based URL Sep 23, 2020
@yeze322
Copy link
Contributor Author

yeze322 commented Sep 23, 2020

@cwhitten Chris where do you think we should append the setting option to? Or make it an extension?

@yeze322
Copy link
Contributor Author

yeze322 commented Sep 23, 2020

@lei9444 Leilei could you help checking if there is any missed case about the selected and focused url params?

@cwhitten
Copy link
Member

@cwhitten Chris where do you think we should append the setting option to? Or make it an extension?

Can you clarify this a bit for me? I'm not sure what you are asking.

@yeze322
Copy link
Contributor Author

yeze322 commented Sep 25, 2020

@cwhitten let me rephrase that.
Do we plan to retire the array index formatted URL, or only use the $designer id URL in PVA env? (I thought it was the latter one.)

If we use $designer id URL in PVA meanwhile keep the array index URL in Composer, then we need an URL option to control which URL format to activate. So where do you suggest we put this URL option at?

I think it could be:

  1. in Settings page, editable when running Composer
  2. in node ENV, determined during build stage
  3. make the URL encoder as an Extension, build PVA version without this encoder extension

@boydc2014
Copy link
Contributor

@cwhitten let me rephrase that.
Do we plan to retire the array index formatted URL, or only use the $designer id URL in PVA env? (I thought it was the latter one.)

If we use $designer id URL in PVA meanwhile keep the array index URL in Composer, then we need an URL option to control which URL format to activate. So where do you suggest we put this URL option at?

I think it could be:

  1. in Settings page, editable when running Composer
  2. in node ENV, determined during build stage
  3. make the URL encoder as an Extension, build PVA version without this encoder extension

Is there a way we support both format at the same time?

@cwhitten
Copy link
Member

@cwhitten let me rephrase that.
Do we plan to retire the array index formatted URL, or only use the $designer id URL in PVA env? (I thought it was the latter one.)
If we use $designer id URL in PVA meanwhile keep the array index URL in Composer, then we need an URL option to control which URL format to activate. So where do you suggest we put this URL option at?
I think it could be:

  1. in Settings page, editable when running Composer
  2. in node ENV, determined during build stage
  3. make the URL encoder as an Extension, build PVA version without this encoder extension

Is there a way we support both format at the same time?

@boydc2014 @yeze322 when we discussed last, we considered a routing layer that will transform an id-based url into an array based url and supporting both

@yeze322
Copy link
Contributor Author

yeze322 commented Sep 29, 2020

The tricky problem here is it's a bi-directional transformation. By supporting both of them, only one format will be activated at last.
The bi-directional transformation means the relationship between URL <-> resource location

  • When parsing URL to resource location (inputting triggers[0].actions[1], focus on correct Action), it's easy to be compatible with both designerid based URL and array index based.
  • However, when generating URL from currect location (e.g. clicking on a Trigger / Action), we have to choose a specifc format, and indeed the browser history will contain only one format.

@yeze322
Copy link
Contributor Author

yeze322 commented Sep 29, 2020

So I think that it's necessary to use a config to define which format is by default.

And at the same time, we can have a solution that inferring current URL format on Composer loaded and then stores it as a global state. This state will instruct which format to use when generating new URL from new resource location.

@cwhitten
Copy link
Member

Hmm. Seems like we would want to avoid this if we can.

What if we changed the routing to only support id-based urls? How would we structure this if we didn't have the array-based constraint?

The more I think about array-based urls and what one expects of a url, it seems less correct. The simple scenario I get tripped up with is:

If I save a deep-link to my clipboard, and someone re-orders the element in the array, the index now points to an incorrect resource

The benefits of array-based don't seem to outweigh this seemingly fundamental problem.

@yeze322
Copy link
Contributor Author

yeze322 commented Sep 29, 2020

@cwhitten That's a reasonable and important scenario. We actually have no strong dependency with the array based URL, I agree with you. To achieve that, all changes required is just as this draft PR's changes.

@cwhitten
Copy link
Member

Can we revisit the query string also? If we don’t support array-based, there is no need for [ or ] in the values

@boydc2014
Copy link
Contributor

Note: The call we made at today's platform sync is keep this PR scoped to transforming the url from index based to id based, and without changing the basic pattern how url is composed.

@yeze322 yeze322 marked this pull request as ready for review September 30, 2020 08:42
@cwhitten cwhitten merged commit cd05173 into main Sep 30, 2020
@cwhitten cwhitten deleted the nav/designer-url branch September 30, 2020 22:48
@cwhitten cwhitten mentioned this pull request Nov 13, 2020
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
* mark out contribution points

* impl designer path encoder/decoder

* use double quotation

* apply encoder / decoder to navigation logic

* fix tslint error

* fix import order

* remove comments

* fix tslint error

* encode breadcrumb path

Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow deep-linking to support $designer.id based lookup
4 participants