make adaptive card library compatible with ie

[atishayv]

Related Issue

Closes : #4855

Description

• Made adaptive-expressions library external dependency of adaptivecards-templating library.
• Made adaptivecards, adaptive-expressions, adaptivecards-templating as external dependecny for adaptivecards-designer.
• Made code changes in designer, adaptive card package to make it compatible with IE.
• Updated README for both adaptive card templating and designer package to install external dependency beforehand.

How Verified

Tested manually in local both adaptive card designer and site in IE to make sure that there are no errors

Microsoft Reviewers: Open in CodeFlow

[ghost]

Hi @atishayv. Thanks for helping make the AdaptiveCards JS renderer + tooling better. As additional verification, once the JS build succeeds, please go to the test site to test out your website/designer changes.

[ghost]

All CLA requirements met.

[atishayv]

@dclaux , @matthidinger The AdaptiveCards-.NET-PR pipeline is failing because of lack of permission.
The nuget command failed with exit code(1) and error(Unable to load the service index for source https://microsoft.pkgs.visualstudio.com/_packaging/b7611912-3467-4769-8b5f-a78215654b19/nuget/v3/index.json. Response status code does not indicate success: 403 (Forbidden - User 'bf2773a8-25bb-49eb-bf61-4a950e22a268' lacks permission to complete this action. You need to have 'ReadPackages'. (DevOps Activity ID: 72E5F74C-F42A-42BB-BBD8-23D47267A0C5)). NuGet.Protocol.Core.Types.FatalProtocolException:

Can you please help me in how to resolve this ?
CC : @jaisanth

[dclaux]

Designer changes look good.

Two things though:

• I don't think we want to release the templating engine solely as a transpiled library. Browsers other than IE don't nee that, and I'd rather us ship "native" JS and only make the transpiled version available to those who really need it
• Your change will work now because it so happens we bundle the Adaptive Expression with our templating library. But that isn't the right thing to do long term; I think sooner than later we'll mark adaptive-expressions as an external, and the host application will be responsible for loading it on its own. My point here is that it is the adaptive-expression library that needs to go through Babel

[atishayv]

Designer changes look good.

Two things though:

• I don't think we want to release the templating engine solely as a transpiled library. Browsers other than IE don't nee that, and I'd rather us ship "native" JS and only make the transpiled version available to those who really need it
• Your change will work now because it so happens we bundle the Adaptive Expression with our templating library. But that isn't the right thing to do long term; I think sooner than later we'll mark adaptive-expressions as an external, and the host application will be responsible for loading it on its own. My point here is that it is the adaptive-expression library that needs to go through Babel

Thanks @dclaux for the suggestion. I am just trying to figure out the right way to do it, so that we dont have to fork out from main branch. I have made the neccessay changes in adaptive expression library here : microsoft/botbuilder-js#2906
to make it compatible with IE and tested it with designer to make sure its working properly.

Once its merged, we can remove the babel and core js dependecny from templating library and make one little change of :

in template engine.ts.

Also, if we dont want to ship the transpiled version with the public library , can we make it configurable and lazy load the expression library(as its a big chunk) so that we can decide based on the config whether to include the transpiled version or not. Also I think lazy loading expression library will help reduce the size of templating library and improve performance.

What do you think ?
CC: @jaisanth, @matthidinger

[dclaux]

@atishayv thanks for the changes.

I think we should look immediately into making adaptive-expressions an external dependency for adaptivecards-templating. This way, the consuming application is responsible for loading adaptive-expressions, and at that point it can choose to load the transpiled version or the native version.

[atishayv]

@atishayv thanks for the changes.

I think we should look immediately into making adaptive-expressions an external dependency for adaptivecards-templating. This way, the consuming application is responsible for loading adaptive-expressions, and at that point it can choose to load the transpiled version or the native version.

Can you please give me some pointers on how are we planning to do it?
Also, is it something that your team will take up ? Or should I work with you to get it done ?

[dclaux]

Making a dependency external should be relatively simple, although I haven't been successful with it myself so I can't be sure. To start with it's about marking it as such by adding it to the externals section of adaptivecards-templating's package.json. That should cause the compiled package to not bundle the external. Then comes the more complicated part, e.g. modifying the designer so that it explicitly loads both adaptivecards-templating and adaptive-expressions - I am not sure about how to handle that part, I have tried in the past but things didn't work.

It would be great if you could look into this. Would that be OK?

[atishayv]

Making a dependency external should be relatively simple, although I haven't been successful with it myself so I can't be sure. To start with it's about marking it as such by adding it to the externals section of adaptivecards-templating's package.json. That should cause the compiled package to not bundle the external. Then comes the more complicated part, e.g. modifying the designer so that it explicitly loads both adaptivecards-templating and adaptive-expressions - I am not sure about how to handle that part, I have tried in the past but things didn't work.

It would be great if you could look into this. Would that be OK?

Hi @dclaux ,
Tried to make expression library as external and everything seems to be working fine. Not sure what problems you were facing. Can you please check if its okay ?
CC : @jaisanth

[dclaux]

@atishayv when I tried it was actually about making the adaptivecards library an external for the designer. Don't know why it didn't work :)

Ok so this is great, however before we can merge into main we need to all be aware of the fact that this is technically a breaking change. Applications that use adaptivecards-templating today might not import adaptive-expressions. When they update to the new version, their app won't work anymore; they will have to explicitly import adaptive-expressions. That includes apps that use the designer, which itself uses adaptivecards-templating. These apps will have to explicitly import adaptive-expressions.

@matthidinger what do you think? I think it's a price worth paying.

@atishayv did you verify that the designer still works?

Also, without pushing things too far, if we make this change I think we should go all in and also do the following:

• Make adaptivecards an external for adaptivecards-designer
• Make adaptivecards-templating an external for adaptivecards-designer
• adaptivecards-designer

[atishayv]

@atishayv when I tried it was actually about making the adaptivecards library an external for the designer. Don't know why it didn't work :)

Ok so this is great, however before we can merge into main we need to all be aware of the fact that this is technically a breaking change. Applications that use adaptivecards-templating today might not import adaptive-expressions. When they update to the new version, their app won't work anymore; they will have to explicitly import adaptive-expressions. That includes apps that use the designer, which itself uses adaptivecards-templating. These apps will have to explicitly import adaptive-expressions.

@matthidinger what do you think? I think it's a price worth paying.

@atishayv did you verify that the designer still works?

Also, without pushing things too far, if we make this change I think we should go all in and also do the following:

• Make adaptivecards an external for adaptivecards-designer
• Make adaptivecards-templating an external for adaptivecards-designer
• adaptivecards-designer

I tried to run localhost preview fetaures html and it was working. Also it was working in this link as well :
https://adaptivecardsci.z5.web.core.windows.net/pr/4927/designer
Let me know if there is any way that I need to verify it :-)

Also I am okay with making adaptive card and templating library external as well. Let me know if I need to do the change as part of this PR

[atishayv]

@atishayv when I tried it was actually about making the adaptivecards library an external for the designer. Don't know why it didn't work :)
Ok so this is great, however before we can merge into main we need to all be aware of the fact that this is technically a breaking change. Applications that use adaptivecards-templating today might not import adaptive-expressions. When they update to the new version, their app won't work anymore; they will have to explicitly import adaptive-expressions. That includes apps that use the designer, which itself uses adaptivecards-templating. These apps will have to explicitly import adaptive-expressions.
@matthidinger what do you think? I think it's a price worth paying.
@atishayv did you verify that the designer still works?
Also, without pushing things too far, if we make this change I think we should go all in and also do the following:

• Make adaptivecards an external for adaptivecards-designer
• Make adaptivecards-templating an external for adaptivecards-designer
• adaptivecards-designer

I tried to run localhost preview fetaures html and it was working. Also it was working in this link as well :
https://adaptivecardsci.z5.web.core.windows.net/pr/4927/designer
Let me know if there is any way that I need to verify it :-)

Also I am okay with making adaptive card and templating library external as well. Let me know if I need to do the change as part of this PR

@dclaux , @matthidinger Can you please suggest what are the action plan for this PR ?
CC : @jaisanth

[ghost]

Hi @atishayv. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along.

[ghost]

Hi @atishayv; Thanks for taking action on your previously stale pull request. Resetting staleness.

[atishayv]

@matthidinger , @dclaux I have updated the PR as per your comments. Let me know if I need to make any more changes for this.
I have tested this PR locally but it would be great if you can test it from your side to check if I am not breaking anything.
CC : @jaisanth

[dclaux]

I haven't tested explicitly, but the changes look good to me. Please do change "optional" into "required" where noted though.

[atishayv]

@dclaux, @matthidinger The command npm run docs is failing in adaptivecards package. Can you please help me understand whats the problem?

[dclaux]

@atishayv what's the error you're getting?

[atishayv]

@atishayv what's the error you're getting?

@dclaux Getting the following error:

adaptivecards: [docs] Loaded plugin typedoc-plugin-markdown
adaptivecards: [docs] Error: Tried to set an option (inputFiles) that was not declared.
adaptivecards: [docs] Error: Tried to set an option (mode) that was not declared.
adaptivecards: [docs] Error: Tried to set an option (excludeNotExported) that was not declared.
adaptivecards: [docs] Error: Tried to set an option (stripInternal) that was not declared.
adaptivecards: [docs] npm ERR! code ELIFECYCLE
adaptivecards: [docs] npm ERR! errno 1
adaptivecards: [docs] npm ERR! adaptivecards@2.1.0 docs: `npx typedoc`
adaptivecards: npm ERR! Exit status 1
adaptivecards: [docs] npm ERR! 
adaptivecards: [docs] npm ERR! Failed at the adaptivecards@2.1.0 docs script.
adaptivecards: [docs] npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
adaptivecards: [docs] 

[dclaux]

Has the typedoc-plugin-markdown package been updated, and there was some sort of a breaking change? Unfrotunately I do not know how it works, so I'm not quite sure how to help.

[atishayv]

how

I have not done any changes related to typedoc-plugin-markdown package update. The PR contains only changes related to update of README.md file.

[dclaux]

The PR contains more than changes to the README.md file. Did you mean this last commit only changes README.md, and are you suggesting that it worked before that last commit?

[atishayv]

The PR contains more than changes to the README.md file. Did you mean this last commit only changes README.md, and are you suggesting that it worked before that last commit?

@dclaux , @matthidinger I am not sure after which commit it stopped working. But I dont think it has anything to do with the changes in this PR.
I tried to clone new repo of adaptive card : https://github.com/microsoft/AdaptiveCards.git
and then i ran following commands :

• cd source/nodejs
• npm install
• npx lerna bootstrap
• npx lerna run build
• cd adaptivecards
• npm run docs

And it failed with the same error :

Can you please help me with this ?

Is it because of the recent update in typedoc library to 0.20.0-beta.4 version ?

[atishayv]

@matthidinger , @dclaux , @paulcam206
Okay, changing the typedoc version to 0.19.2 and modifying the mode value in typedoc.json file fixes the issue

Let me know if I need to do these changes in a separate PR ?

[dclaux]

Good. I think this change can go into this same PR.

In terms of merging the PR - we've branched our 20.10 release a couple days ago, so anything that goes into main now will be part of the 20.11 release, including this change. I think that's good as it will give us a full month to make sure things continue working as expected and communicate the breaking change with partners.

So @matthidinger, pending the NodeJS checks passing (they don't at this moment) I think we can merge this PR. Would you agree?

[dclaux]

/azp run

[azure-pipelines]

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

[atishayv]

@dclaux , @matthidinger i have updated the pr. Now all the checks are passing. Let me know if there is anymore changes needed to merge it.

[atishayv]

@dclaux Can we merge this ?

[dclaux]

I just updated the branch, will merge once the tests have passed.

[matthidinger]

@atishayv when I tried it was actually about making the adaptivecards library an external for the designer. Don't know why it didn't work :)
Ok so this is great, however before we can merge into main we need to all be aware of the fact that this is technically a breaking change. Applications that use adaptivecards-templating today might not import adaptive-expressions. When they update to the new version, their app won't work anymore; they will have to explicitly import adaptive-expressions. That includes apps that use the designer, which itself uses adaptivecards-templating. These apps will have to explicitly import adaptive-expressions.
@matthidinger what do you think? I think it's a price worth paying.
@atishayv did you verify that the designer still works?
Also, without pushing things too far, if we make this change I think we should go all in and also do the following:

• Make adaptivecards an external for adaptivecards-designer
• Make adaptivecards-templating an external for adaptivecards-designer
• adaptivecards-designer

I tried to run localhost preview fetaures html and it was working. Also it was working in this link as well :
https://adaptivecardsci.z5.web.core.windows.net/pr/4927/designer
Let me know if there is any way that I need to verify it :-)
Also I am okay with making adaptive card and templating library external as well. Let me know if I need to do the change as part of this PR

@dclaux , @matthidinger Can you please suggest what are the action plan for this PR ?
CC : @jaisanth

@atishayv at some point the Designer seems to have broken, per the thread I added you to a few minutes ago. And the PR link is no longer working.

https://adaptivecardsci.z5.web.core.windows.net/pr/4927/designer

We have a community call that relies on this demo working Thursday at 9am if there is any way you can dig in and see what may have broken along the way that would be much appreciated!

[shalinijoshi19]

@atishayv it appears the samples no longer render - both on localhost as well as on our latest CI website which is running as yet unshipped bits from main/release/20.12 branch - issue #5193 . Thoughts here at all ? Also was the concern that @matthidinger had raised earlier with this PR ever addressed prior to merging again? Thanks! @golddove who's been helping dig into this ahead of our scheduled release today.

[golddove]

Switching adaptive-expressions from a dependency to a peerdependency means the consumer has to now explicitly include it separately, instead of getting it automatically with adaptivecards-templating. This is a breaking change - so probably shouldn't go out in a minor version. @shalinijoshi19, @dclaux, @matthidinger, thoughts?

[atishayv]

@shalinijoshi19 yes the issue that @matthidinger has raised before was fixed here
#5072

Let me debug the issue related to samples not working and get back