Skip to content

feat: better error message when no options are provided #118

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

Merged

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Mar 19, 2022

Summary

adds a detailed error message when no options are provided to a polyfill plugin as requested in #17

Details

  • previously, if polyfills were ran without options, it would just
    throw ".method must be a string", which is not very intuitive

    • now, say that the plugin requires options, give an example of a
      config with options, and link to the docs for further reference
  • add a test for this new error as well

Review Notes

I haven't used Flow before (only TS), so let me know if I should have done some flow typing anywhere.
(for context: I was thinking I could put a type-guard or something around isEmpty, but given that that's a more advanced TS feature, I was not really sure how to do so in Flow)

References

Fixes #17

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I haven't used Flow before (only TS), so let me know if I should have done some flow typing anywhere

Tbh I should probably convert this repository to TS similarly to how we did it for the main babel repository 😅

@agilgur5
Copy link
Contributor Author

Tbh I should probably convert this repository to TS similarly to how we did it for the main babel repository 😅

Was there a tool used for this? Or could you point me to the relevant issues/PRs there?

I could try taking a stab at that conversion here since this repo is significantly smaller and I'm a familiar with a decent chunk of it now 🙂

- previously, if polyfills were ran without options, it would just
  throw ".method must be a string", which is not very intuitive
  - now, say that the plugin requires options, give an example of a
    config with options, and link to the docs for further reference

- add a test for this new error as well
@agilgur5 agilgur5 force-pushed the feat-better-error-message-no-options branch from 42a4e3c to 57d501e Compare March 20, 2022 16:05
@nicolo-ribaudo
Copy link
Member

We started with https://github.com/zxbodya/flowts, and then made different manual adjustments. The TS setup in the babel/babel repository is more complicated than what we need for this one: probably a tsconfig.json like this would be enough

{
  "compilerOptions": {
    "target": "esnext",
    "module": "esnext",
    "lib": [
      "esnext"
    ],
    "noEmit": true,
    "moduleResolution": "node",
    "esModuleInterop": true,
    "isolatedModules": true,
    "skipLibCheck": true,
    "resolveJsonModule": true
  },
  "include": ["./packages/*/src/**/*.ts"],
  "compilerOptions": {
    "paths": {
      "@babel/helper-define-polyfill-provider": [
        "./packages/babel-helper-define-polyfill-provider/src"
      ]
    }
  }
}

@nicolo-ribaudo nicolo-ribaudo merged commit 5b2cb96 into babel:main Mar 20, 2022
@agilgur5
Copy link
Contributor Author

Thanks for the details! I found the main PR babel/babel#11578 in the core repo (and linked PRs) as well as the "Flow -> TS" label to get a sense of how the migration/conversion was structured (each plugin split, rename then convert, build tooling PRs, etc).

I'll try to get an initial PR (or multiple) out for this repo this week to start off the conversion here!
May file a new issue to keep track of things if necessary

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 22, 2022

A single PR will probably be easier and require less work! This repository is small enough that we can do it.

@agilgur5 agilgur5 mentioned this pull request Mar 26, 2022
7 tasks
@agilgur5
Copy link
Contributor Author

agilgur5 commented Mar 26, 2022

Gotcha, I wasn't sure what the changes would look like, so I didn't want to assume that it would be easy to do/organize/review as one PR.

@nicolo-ribaudo See #121 for a near-complete draft/WIP PR for the conversion. Took several hours over a few days to get about 90%+ there. Only a handful of things remaining -- some of the remaining type errors may require your input, but I'll try to take a second look at them later

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.

Better error message when not providing options
2 participants