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

port: Better binary data support in Expression/LG functions #3390

Merged
merged 10 commits into from
Mar 17, 2021

Conversation

Danieladu
Copy link
Contributor

@Danieladu Danieladu commented Mar 11, 2021

Fixes #3385

Details

Replace Buffer operations with TextDecoder/TextEncoder and other libraries.(btoa-lite)

Support binary input for prebuilt function string

Now, string would support converting a binary object into a normal string with UTF-8 decoding.
For example:
string(binary('hello')) -> 'hello'

Refine fromFile

Originally, fromFile accepts only one parameter that refers to the file path. The step is as follows:

  • Find file
  • Read file with string stream and get the string result
  • Evaluate the text

In this PR, we introduce the second parameter of fromFile.
Its value could be: evaluated (default), raw or binary

So the change of this function is:
fromFile(path: string) ->
fromFile(path: string, format: 'evaluated'(default) | 'raw' | 'binary')

raw format

In some situations, users do not want to evaluate the text, and just want to keep the original content text.

Example:
There is a file, xx.txt, content is:

hi ${name}

Original, user could use fromFile('xx.txt') to get hi jack, hi Lucy,etc. according to the memory/properties passed in.
But there is no way to get the original text.
Now, user could use fromFile('xx.txt', 'raw') to get hi ${name}

binary format

In another situation, the user wants to get the binary result of the file.

Example:
string(fromFile('xx.txt', 'binary')) => 'hi ${name}'
If you want to evaluate it directly, just append expandText function:
expandText(string(fromFileBinary('xx.txt'))) => 'hi Jack', 'hi Lucy' ...

Another example for Bot builder:
For Bot response, if the user wants to send a local image to the client, here is the solution:

# attachmentTemplate
[Attachment
    name = myImageName
    contentType = image/png
    contentUrl = data:image/png;base64,${${base64(fromFile(xx/name.png, 'binary'))}}
]

@Danieladu Danieladu requested review from joshgummersall, stevengum and a team as code owners March 11, 2021 05:19
@Danieladu Danieladu changed the title [PORT]Better binary data support in Expression/LG functions port: Better binary data support in Expression/LG functions Mar 11, 2021
@Danieladu
Copy link
Contributor Author

Additional change: Migrate all myget feed package to yarnpkg registry in yarn.lock

@Danieladu
Copy link
Contributor Author

Danieladu commented Mar 11, 2021

Another question:
Here is the story line: I want to drop the use of Buffer(node only), instead with third-party library "atob-lite" and "btoa-lite" and TextDecoder/TextEncoder.
Here are the concerns and thinking:
TextDecoder/TextEncoder is introduced into node.js during v8.3, but as a global variable till v11.0, so, in node 10.(Which we should support currently in CI pipeline.), May apply this code:

const util = require('util');
const encoder = new util.TextEncoder();

But in node >= 11 and browser(> IE 11), the below code is enough:

const encoder = new TextEncoder();

There are several options:

  1. Dynamic check the environment and node version, to apply the implements for different platform and versions.
  2. Upgrade our minimum supported node from 10 to 12, which is consistent with Composer.
  3. Use some third-Party libraries, like @sinonjs/text-encoding
  4. Keep Buffer operation.
    @joshgummersall what do you think?

@joshgummersall
Copy link
Contributor

joshgummersall commented Mar 11, 2021

It looks like require('util').TextEncoder still works in Node 12+

~ > node -v
v14.16.0
~ > node -e 'console.log(require("util").TextEncoder)'
[class TextEncoder]

I would just use that as it's backward-compatible.

EDIT: Ah - the browser! Right. The standard way of using globals and then shimming is to check typeof X !== 'undefined' so I would suggest doing that and falling back to require('util').TextEncoder if the check fails.

@coveralls
Copy link

coveralls commented Mar 17, 2021

Pull Request Test Coverage Report for Build 659580814

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 54 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.08%) to 85.023%

Files with Coverage Reduction New Missed Lines %
libraries/adaptive-expressions/src/builtinFunctions/string.ts 2 88.24%
libraries/adaptive-expressions/src/functionUtils.internal.ts 8 94.69%
libraries/botbuilder-lg/src/evaluator.ts 14 92.02%
libraries/botbuilder-lg/src/expander.ts 30 74.02%
Totals Coverage Status
Change from base Build 658708963: -0.08%
Covered Lines: 18792
Relevant Lines: 21054

💛 - Coveralls

@Danieladu Danieladu merged commit 3022f79 into main Mar 17, 2021
@Danieladu Danieladu deleted the hond/fromFile branch March 17, 2021 02:37
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.

port: Better binary data support in Expression/LG functions (#5118)
3 participants