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

Expose ~JSX namespace~ HResult globally #18

Closed
4 tasks done
alecmev opened this issue Aug 22, 2022 · 10 comments
Closed
4 tasks done

Expose ~JSX namespace~ HResult globally #18

alecmev opened this issue Aug 22, 2022 · 10 comments
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done 🦋 type/enhancement This is great to have

Comments

@alecmev
Copy link

alecmev commented Aug 22, 2022

Initial checklist

Problem

I'm using TypeScript with "moduleResolution": "Node16" and JSX, and I want to have explicit return types. Currently I do this:

import type { Root, Element } from "hast";
const Foo = (): Root | Element => <div />;

Instead, I'd like to be able to do this:

const Foo = (): JSX.Element => <div />;

I know it's an alias of HResult, which can be imported from lib/core, but it isn't idiomatic, and lib/core currently isn't exposed, so it doesn't work in this module resolution mode.

Solution

@types/react puts JSX in declare global. Could hastscript do that too?

Alternatives

Export lib/core.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Aug 22, 2022
@wooorm
Copy link
Member

wooorm commented Aug 22, 2022

Sounds like something @remcohaszing has ideas on!

@remcohaszing
Copy link
Member

The node16 resolution type is the correct one for native ESM, and therefor for unified packages internally. Unfortunately not all of the ecosystem is compatible yet (davidbonnet/astring#646). This needs to be resolved first, then I can look at how all of this affects hastscript.

A quick history on this topic:

  • At first the TypeScript JSX namespace had to be global.
    declare global {
      namespace JSX {
        // …
      }
    }
    During this time the React types were created.
  • At some point TypeScript allowed specifying local JSX namespaces
    declare function h();
    
    namespace h.JSX {
      // …
    }
    This allows people to mix multiple JSX implementations in different filed within the same project 🤯 So these two files could live in the same project:
    // react-hello.tsx
    import React from 'react'
    
    const Hello = () => <div>Hello</div>
    // hastscript-hello.tsx
    /* @jsx h */
    import { h } from 'hastscript'
    
    const Hello = () => <div>Hello</div>
  • Then the automatic runtime was released.

As far as I know, this requires a global JSX namespace. However, this issue suggests this is incorrect. If this is indeed incorrect, then IMO it is the desired behaviour, because it would allow to mix multiple JSX implementationg using JSX comments.

I do think it would be nice to expose the HResult type though. I totally agree it makes sense to use this type, i.e. for explicit return types as the OP suggests.

import { HResult } from 'hastscript'

const Foo = (): HResult => <div />;

@alecmev
Copy link
Author

alecmev commented Aug 22, 2022

Thanks, that makes sense! So I went ahead and checked if anybody has reported this in TypeScript, since this feels like their responsibility, and I found something even better: microsoft/TypeScript#41330 (comment) Here's how Preact implemented it: https://github.com/preactjs/preact/pull/2811/files Edit: And I see that it's already exported, so why doesn't it work? Edit 2: Ah, likely because it isn't re-exported here and here and here. Edit 3: No, I still don't understand why this isn't sufficient:

// svg/jsx-runtime.d.ts

export * from '../lib/runtime-svg.js'

// lib/runtime-svg.d.ts

export * from './jsx-automatic.js'

// lib/jsx-automatic.d.ts

export namespace JSX ...

Edit 4: Okay, finally found the definitive answer to this: microsoft/TypeScript#47072 (comment) The JSX namespace is an implementation detail, not meant to be public. So, in the end, import { HResult } is the way to go.

@wooorm
Copy link
Member

wooorm commented Aug 23, 2022

As far as I understand the above comments, then it would be bad to do this?
And exposing HResult is an adequate solution?

@alecmev
Copy link
Author

alecmev commented Aug 23, 2022

it would be bad to do this?

Yep! Looks like JSX isn't meant to be accessed ever.

exposing HResult is an adequate solution?

I believe so. The name is a little cryptic, but it's not worth a refactor, and breaking existing consumers.

@wooorm
Copy link
Member

wooorm commented Aug 24, 2022

I’m up for exposing HResult, I don’t see downsides.
@alecmev Interested in working on a PR?

@wooorm wooorm changed the title Expose JSX namespace globally Expose ~JSX namespace~ HResult globally Aug 24, 2022
@wooorm wooorm added 🦋 type/enhancement This is great to have ☂️ area/types This affects typings 👍 phase/yes Post is accepted and can be worked on labels Aug 24, 2022
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Aug 24, 2022
@github-actions

This comment has been minimized.

@wooorm wooorm closed this as completed in 464b23d Oct 10, 2022
@github-actions

This comment has been minimized.

@wooorm
Copy link
Member

wooorm commented Oct 10, 2022

Released in 7.1.0!

I noticed that HChild, HProperties, were also exposed as Child and Properties, so I used Result as the type name for this

@wooorm wooorm added the 💪 phase/solved Post is done label Oct 10, 2022
@github-actions github-actions bot removed the 👍 phase/yes Post is accepted and can be worked on label Oct 10, 2022
@alecmev
Copy link
Author

alecmev commented Dec 22, 2022

Sorry for the radio silence, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done 🦋 type/enhancement This is great to have
Development

No branches or pull requests

3 participants