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

Make defaultReporter available as a named export #140

Merged
merged 3 commits into from
Mar 10, 2021
Merged

Conversation

af
Copy link
Owner

@af af commented Mar 3, 2021

Potential solution for #137 (comment)

@af af mentioned this pull request Mar 3, 2021
@af af force-pushed the export-default-reporter branch from 06536ee to 10504cf Compare March 3, 2021 03:30
@kachkaev
Copy link
Contributor

kachkaev commented Mar 3, 2021

Awesome, thanks for this PR @af! It will will definitely save me from repeating code in a couple of projects! To be honest, the only way I usually override the reporter is making it throw instead of exiting. Perhaps, the library could expose something like reporterThatThrows and reporterThatExits? Names are up to you of course.

At the moment, ‘reporter that throws’ looks like this for me. It can be more elegant if both reporters could be based on generateReport(stuff) => string – there would be no need to stub console.error and process.exit.

export const customEnvalidReporter: typeof defaultReporter = (opts) => {
  const originalConsoleError = console.error;
  const originalProcessExit = process.exit;

  let errorOutput = "\n";
  console.error = (output) => {
    errorOutput += output;
  };
  process.exit = () => undefined as never;

  defaultReporter(opts);

  process.exit = originalProcessExit;
  console.error = originalConsoleError;

  if (errorOutput.length > 1) {
    throw new Error(errorOutput);
  }
};

src/index.ts Outdated
@@ -3,3 +3,4 @@ export * from './errors'
export * from './middleware'
export * from './types'
export * from './validators'
export { default as defaultReporter } from './reporter'
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is not related to the PR directly, but might help you in the future a bit. I stopped using default exports for anything except for React components and my DX became noticeably better.

When re-exporting files from a directly, I can now always do:

export * from "./a";
export * from "./b";
export * from "./d";

If there is a file that exports both ‘private’ and ‘public’ things, I split it into two and don’t re-export stuff from the ‘private’ file. In the example above, it would be "./c".

Here’s a blog post on this topic:
https://basarat.gitbook.io/typescript/main-1/defaultisbad

@SimenB
Copy link
Collaborator

SimenB commented Mar 3, 2021

To be honest, the only way I usually override the reporter is making it throw instead of exiting. Perhaps, the library could expose something like reporterThatThrows and reporterThatExits?

I still (#101 (comment)) think we should throw rather than exit, but not just for browser compat. In practice it's the same thing for the consumer unless they explicitly catch and ignore the error. But then that's their choice.

That said, exporting the reporter doesn't seem like it should hurt, so not really a comment on this PR as much as the use case 😀

@af
Copy link
Owner Author

af commented Mar 6, 2021

@kachkaev Added a couple commits to use a named export and also be able to pass in { onError, logger } as a second defaultReporter arg. This way you can customize the error behavior and logging transport to your heart's content. Let me know if you see any issues with this, or if it wouldn't address your use case.

@SimenB Maybe it's just me being unreasonably picky, but I don't like having the unnecessary traceback when throwing in the reporter. It certainly would be simpler to just throw though. Anyways, you should be able to always throw with this latest commit if you wish :)

@kachkaev
Copy link
Contributor

kachkaev commented Mar 6, 2021

I like how it looks! Thanks so much for making the changes!

@af af merged commit 3afb7ea into main Mar 10, 2021
@af af deleted the export-default-reporter branch March 10, 2021 05:07
tuannm151 pushed a commit to BSSCommerce/shopify-envalid that referenced this pull request Jul 1, 2024
* Make defaultReporter available as a named export
* Add extra defaultReporter options, use named export
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.

3 participants