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

Convert to TypeScript #10

Merged
merged 12 commits into from
Aug 11, 2023
Merged

Convert to TypeScript #10

merged 12 commits into from
Aug 11, 2023

Conversation

IanVS
Copy link
Contributor

@IanVS IanVS commented Aug 11, 2023

This is a first pass at converting the library to TypeScript. A few types should be strengthened, but at least it builds and typechecks successfully.

I had to change up the logic a bit in extractRouteArguments, because the way it was behaving before was difficult/impossible to do with type safety.

This is building for esm and cjs, including separate type definitions, using tsup. I kept a pretty low target, ES2015, for now, to ensure broad compatibility.

type RestRequest,
type SetupWorkerApi,
} from 'msw';
import type { Server } from 'miragejs';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This type isn't actually used, but because of the way the mirage types are defined, the /server types won't be found unless something is imported from the top level, in which case the other modules are cached and used as well.

@@ -25,6 +29,13 @@ type HTTPVerb =
| 'options'
| 'head';

type BaseHandler = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had thought about this. Really baseHandler should be defined in mirageJS proper and this implement it to ensure any changes in the handler in mirage are respected by the pretender/msw modules.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also it would avoid the need to create these again for the pretender repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a great point. In fact, I think the handlers defined in mirage are incorrect:

    /** Handle a GET request to the given path. */
    get<Response extends AnyResponse>(
      path: string,
      handler?: RouteHandler<Registry, Response>,
      options?: HandlerOptions
    ): void;

That's only one format, but they can also be just options, or just a handler, or a number of other formats (unfortunately).

I think there's still quite a bit of cleanup / deduplication to be done, but hopefully this is at least a baby step in the right direction.

@IanVS IanVS merged commit 2e214af into master Aug 11, 2023
2 checks passed
@IanVS IanVS deleted the ts branch August 11, 2023 21:20
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.

None yet

2 participants