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

Switch from namespace syntax to declare module syntax #33

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

abcd-ca
Copy link

@abcd-ca abcd-ca commented Mar 4, 2024

The namespaces syntax was causing Vite to fail to compile. I changed the emitter to generate declare module syntax instead and now Vite is happy. The import syntax in my Typescript app that consumes the generated types look nicer now too.

Generated backendTypes.d.ts before:

export default backendTypes;
export namespace backendTypes {
    export type U32 = number;
    export type Opening = {
        "id": backendTypes.U32;

        /**
         * if `false` then it's tomorrow
         */
        "is_today": boolean;

        /**
         * Seconds
         */
        "duration": backendTypes.U32;
        "start_at": string;
        "end_at": string;
    };
}

Import syntax before:

import backendTypes from '../../model/backendTypes'
import Opening = backendTypes.Opening

Generated backendTypes.d.ts after:

declare module "backendTypes" {
    export type U32 = number;
    export type Opening = {
        "id": U32;

        /**
         * if `false` then it's tomorrow
         */
        "is_today": boolean;

        /**
         * Seconds
         */
        "duration": U32;
        "start_at": string;
        "end_at": string;
    };
}

Import syntax before:

import { Clinic } from 'backendTypes'

@abcd-ca abcd-ca changed the title Fix/namespace to module Switch from namespace syntax to declare module syntax Mar 4, 2024
@abcd-ca
Copy link
Author

abcd-ca commented Mar 4, 2024

Hi, let me know what you think; I can fix the tests and maybe the lint step if that's my change that the Lint check is complaining about

@dbeckwith
Copy link
Owner

dbeckwith commented Mar 10, 2024

I'd like to understand a little more about the problem you were having with Vite and exactly why this solves it. Until I understand that problem I'm hesitant to make this change as I don't see what real advantages this approach has.

Semantically, the types being generated by this library do not describe an actual JavaScript module, as the use of declare module would imply. The types are only meant to be used purely as types to describe some data that you might receive externally. So semantically, it makes more sense to me that you're just defining a bunch of types with namespaces applied for organizational purposes, rather than defining types that describe a JavaScript module.

Another thing it sounded like you wanted to change was the ergonomics of importing types: import { Clinic } from 'backendTypes' rather than import backendTypes from '../../model/backendTypes'; backendTypes.Clinic. I believe this is already mostly possible with the way the library is currently. Firstly, there is an option when generating the types to omit the root namespace entirely, meaning the types will be exported directly from the top level, allowing import { Clinic }. Secondly, for making the import file path simpler, this is something I've solved in my Typescript projects that use Webpack by using aliases along with the paths Typescript config option, and I would expect other build systems to have something similar.

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