Skip to content

feature request: option to disable TEXT_ENCODING env check #219

Closed
#221
Closed
feature request: option to disable TEXT_ENCODING env check#219
#221

Description

@andykais
andykais
opened on Jan 11, 2023

Hi, I am using this library in deno, and I noticed part of the encode/decode flow checks for the TEXT_ENCODING environment variable. It seems like this is a useful env var for testing, but I do not need it in production. In deno, I have to allow access to this var explicitly, and will have to tell users of my library to do the same, which feels confusing if this is in fact a testing var. I was wondering if it would be possible to send along a flag like

import * as msgpack from 'npm:@msgpack/msgpack'

msgpack.decode(buffer, { env_check: false })
// or
msgpack.decode(buffer, { text_encoding: false })

for reference, this is the warning deno will share when I do not allow access to env with msgpack.

./test/functional.test.ts (uncaught error)
error: PermissionDenied: Requires env access to "TEXT_ENCODING", run again with the --allow-env flag
    return Deno.env.get(name);
                    ^
    at Object.getEnv [as get] (deno:runtime/js/30_os.js:86:16)
    at denoEnvGet (https://deno.land/std@0.171.0/node/_process/process.ts:38:21)
    at Object.get (https://deno.land/std@0.171.0/node/_process/process.ts:59:24)
    at Object.<anonymous> (file:///home/andrew/.cache/deno/npm/registry.npmjs.org/@msgpack/msgpack/2.8.0/dist/utils/utf8.js:7:177)
    at Object.<anonymous> (file:///home/andrew/.cache/deno/npm/registry.npmjs.org/@msgpack/msgpack/2.8.0/dist/utils/utf8.js:168:4)

None of this is really a blocker for using this library, I just believe it could make usage a bit more friendly for the deno audience :)

Activity

  1. gfx commented on Jan 11, 2023

    @gfx
    gfx
    on Jan 11, 2023
    Member

    Thank you for your report. That's a good point. I would rather like to surround the env access with try { ... } to ignore permission errors in Deno (or any other JavaScript runtimes). Does it work for you?

  2. andykais commented on Jan 11, 2023

    @andykais
    andykais
    on Jan 11, 2023
    Author

    hmm that will still prompt the user for access to the var (unless they specifically pass --no-prompt to deno, which will just error out). What is the purpose of the env var?

  3. gfx commented on Jan 16, 2023

    @gfx
    gfx
    on Jan 16, 2023
    Member

    Ah, that's a good point. So I'm willing to implement the feature to omit env accesses.

    FYI the env accesses are required to test the cases where TextEncoder is not implemented (https://caniuse.com/textencoder it's only IE!). Because I'm dropping IE support, removing the branch where TextEncoder is not implemented might be good enough.

  4. andykais commented on Jan 18, 2023

    @andykais
    andykais
    on Jan 18, 2023
    Author

    👍 well that would be good for me, I trust you will do whats best for your library! You could always drill down an option bag rather than use env vars like I had suggested in the initial message if you dont want to remove it entirely (I feel like nodejs still doesnt have a TextEncoder class?)

  5. gfx
    closed this as completedin #221on Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions