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

Ensure default values don't go through validation, fix type narrowing from defaults #176

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

af
Copy link
Owner

@af af commented Nov 3, 2021

2 fixes:

  • Default values are currently being passed through validators, and this actually breaks the json validator because it's calling JSON.parse() on an object. Probably a regression in v7
  • The changes in Fix issue #139, validator return type is too broad #146 are nice for having more specific union/literal types when possible (eg when using choices). However it appears to have caused a regression where providing a default value would cause the inferred output type to be that literal value rather than a broader type like string. The fix here is kinda hacky but should be suitable for most use cases. choices still causes the output type to be narrowed to a union type of its possible options

@@ -1,3 +1,13 @@
// Hacky conditional type to prevent default/devDefault from narrowing type T to a single value.
// Ideally this could be replaced by something that would enforce the default value being a subset
// of T, without affecting the definition of T itself
Copy link
Owner Author

@af af Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely interested in any better approaches here :(

Before this PR, out = cleanEnv(env, { X: str({ default: 'abc' }) }) will cause out.X to be inferred as type 'abc', rather than string, so this is an incomplete solution to fix that

// default value should be passed through without running through JSON.parse()
expect(cleanEnv({}, {
FOO: json({ default: { x: 999 } })
})).toEqual({ FOO: { x: 999 } })
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this test for this PR fixing this issue

@kachkaev
Copy link
Contributor

kachkaev commented Feb 3, 2022

@af @SimenB 👋

It‘d be great to see this PR merged 🙏

Do I understand the change correctly?

const env = envalid.cleanEnv({
  XYZ: envalid.str({ choices: ["a", "b"] }),
});

typeof env.XYZ // before: string, after "a" | "b"

@af
Copy link
Owner Author

af commented Feb 4, 2022

@kachkaev thanks for the ping, I'd forgotten about this 😅 I believe the fix for the typing issue you found was already merged in #146 , but it hasn't been published yet as I was hoping to get this PR in as well (due to the regression mentioned above).

If you could give this branch a test for your use case and report back that'd be really helpful! 🙏 Then I'll do another check and if all goes well, merge and publish

@kachkaev
Copy link
Contributor

kachkaev commented Feb 5, 2022

I just tried this branch in a project with 12 envalid calls in various script files. Here are my steps:

  1. Open package.json and change "envalid": "^7.2.2" to "envalid": "github:af/envalid#fix-defaults"

  2. Run yarn install

  3. Ensure yarn lint:tsc is not working (this is expected because node_modules/envalid does not have dist)

  4. Recreate dist

    cd node_modules/envalid
    
    npm install
    npm run build
    
    cd ../..
  5. Run yarn lint and observe no errors! 🎉

Type narrowing for choices works like a charm:

envalid.str({
  choices: ["a", "b", "c"] as const,
  default: "a"
})

Output is of type "a" | "b" | "c", which is great! Adding as const does the trick, no str<...> is necessary.

The only way I could ‘break’ it was this:

envalid.str({
  choices: ["a", "b", "c"] as const,
  default: "d"
})

envalid.str<"a", "b", "c">({
  choices: ["a", "b", "c"],
  default: "d"
})

Output is still of type "a" | "b" | "c" despite "d" and there are not TS errors. I guess there will be a runtime error though, so it’s fine as is.

Thanks a lot for this change, I really like it! 🚀

@af af merged commit ef5a895 into main Feb 7, 2022
@af af deleted the fix-defaults branch February 7, 2022 03:50
@af
Copy link
Owner Author

af commented Feb 7, 2022

Thanks @kachkaev for taking that for a spin! Just published 7.3.0-beta.1 with this change, let me know if you run into any issues with it

@kachkaev
Copy link
Contributor

kachkaev commented Feb 7, 2022

Thanks for releasing 7.3.0-beta.1 @af! I tried it, here is the result: kachkaev/tooling-for-how-old-is-this-house#29

  • It works, which is great!

  • No new issues from ESLint or TypeScript linters (and choices: ["a", "b", "c"] as const does the narrowing!)

  • I had to replace named imports with the default one:

    import * as envalid from "envalid";
    // ↓
    import envalid from "envalid";

    If I don’t do this, calls like envalid.str result TypeError: envalid.str is not a function. Calling import { str } from "envalid" does not work either:

    import { str } from "envalid";
            ^^^
    SyntaxError: Named export 'str' not found. The requested module 'envalid' is a CommonJS module, which may not support all module.exports as named exports.
    CommonJS modules can always be imported via the default export, for example using:
    
    import pkg from 'envalid';
    const { str } = pkg;
    

There are subtle differences in dist syntax (before / after). Not sure where they are coming from, but can they explain this breaking change? Note that the project I’ve shared above uses native ESM modules, i.e. I am importing a CommonJS module from a ESM one.


import * as envalid from "envalid";

console.log(envalid);

7.2.2:

[Module: null prototype] {
  EnvError: [Function: EnvError],
  EnvMissingError: [Function: EnvMissingError],
  __esModule: true,
  accessorMiddleware: [Function: accessorMiddleware],
  applyDefaultMiddleware: [Function: applyDefaultMiddleware],
  bool: [Function: bool],
  cleanEnv: [Function: cleanEnv],
  customCleanEnv: [Function: customCleanEnv],
  default: {
    __esModule: true,
    cleanEnv: [Getter],
    customCleanEnv: [Getter],
    testOnly: [Getter],
    EnvError: [Getter],
    EnvMissingError: [Getter],
    strictProxyMiddleware: [Getter],
    accessorMiddleware: [Getter],
    applyDefaultMiddleware: [Getter],
    makeValidator: [Getter],
    bool: [Getter],
    num: [Getter],
    str: [Getter],
    email: [Getter],
    host: [Getter],
    port: [Getter],
    url: [Getter],
    json: [Getter],
    defaultReporter: [Getter]
  },
  defaultReporter: [Function: defaultReporter],
  email: [Function: email],
  host: [Function: host],
  json: [Function: json],
  makeValidator: [Function: makeValidator],
  num: [Function: num],
  port: [Function: port],
  str: [Function: str],
  strictProxyMiddleware: [Function: strictProxyMiddleware],
  testOnly: [Function: testOnly],
  url: [Function: url]
}

7.3.0-beta.1:

[Module: null prototype] {
  __esModule: true,
  default: {
    __esModule: true,
    cleanEnv: [Getter],
    customCleanEnv: [Getter],
    testOnly: [Getter],
    EnvError: [Getter],
    EnvMissingError: [Getter],
    strictProxyMiddleware: [Getter],
    accessorMiddleware: [Getter],
    applyDefaultMiddleware: [Getter],
    makeValidator: [Getter],
    bool: [Getter],
    num: [Getter],
    str: [Getter],
    email: [Getter],
    host: [Getter],
    port: [Getter],
    url: [Getter],
    json: [Getter],
    defaultReporter: [Getter]
  }
}

@af
Copy link
Owner Author

af commented Feb 7, 2022

Thanks for that test and the details! 🙏 7.2.2 was built using typescript 4.3 and the beta was with 4.5, so that must be the difference. I haven't followed the ESM changes too closely but it must be related to that. I might not have time to dig in for the next couple days but let me know if you find any clues or tsconfig settings that might prevent this backwards incompatibility

@kachkaev
Copy link
Contributor

kachkaev commented Feb 7, 2022

Seems like we are dealing with a breaking change in TS 4.4: microsoft/TypeScript#45813.

I probably managed to fix it locally by setting importHelpers to false in envalid’s tsconfig.json. I swapped my project’s node_modules/envalid/dist with the results of yarn prepare for testing. Writing

import * as envalid from "envalid";

console.log(envalid);

in project code produces the same output as for envalid 7.2.2 (which used TS 4.3). However, dist/index.js looks very different to 7.2.2 – I’m not sure if this can break anything elsewhere:

"use strict";
var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    Object.defineProperty(o, k2, { enumerable: true, get: function() { return m[k]; } });
}) : (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    o[k2] = m[k];
}));
var __exportStar = (this && this.__exportStar) || function(m, exports) {
    for (var p in m) if (p !== "default" && !Object.prototype.hasOwnProperty.call(exports, p)) __createBinding(exports, m, p);
};
exports.__esModule = true;
__exportStar(require("./envalid"), exports);
__exportStar(require("./errors"), exports);
__exportStar(require("./middleware"), exports);
__exportStar(require("./types"), exports);
__exportStar(require("./validators"), exports);
__exportStar(require("./reporter"), exports);
//# sourceMappingURL=index.js.map

There is a planned fix in TS 4.6: microsoft/TypeScript#47070. I installed typescript 4.6.0-beta in my local copy of envalid repo, set "importHelpers": true, then ran yarn prepare. dist/index.js matched itself as of v7.2.2.

I don’t know how comfortable you with using 4.6.0-beta when cutting 7.3.0. Happy for you to do that or set "importHelpers": false – I’ll reinstall a new canary version and see if it works. Rolling back to TS ~4.3 can also be an option. AWS SDK folks did that: aws/aws-sdk-js-v3#2782 + aws/aws-sdk-js-v3#3217.

af added a commit that referenced this pull request Feb 9, 2022
@af
Copy link
Owner Author

af commented Feb 9, 2022

Amazing, thank you @kachkaev. Just published envalid@7.3.0-beta.2 using TS 4.6 beta. ts-jest warns but seems to work fine. Let me know if that does the trick!

@kachkaev
Copy link
Contributor

kachkaev commented Feb 9, 2022

Great! Thanks for releasing the new beta! It works like a charm in kachkaev/tooling-for-how-old-is-this-house#29 – no breaking changes 🔥 I will keep the PR open for now and will merge it when the new stable version is out.

@af
Copy link
Owner Author

af commented Feb 13, 2022

Sounds good, my plan is to wait for 4.6 stable, then update and test, then release a new stable envalid assuming everything looks ok

tuannm151 pushed a commit to BSSCommerce/shopify-envalid that referenced this pull request Jul 1, 2024
tuannm151 pushed a commit to BSSCommerce/shopify-envalid that referenced this pull request Jul 1, 2024
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.

2 participants