hastscript icon indicating copy to clipboard operation
hastscript copied to clipboard

Expose ~JSX namespace~ `HResult` globally

Open alecmev opened this issue 2 years ago • 7 comments

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.

alecmev avatar Aug 22 '22 10:08 alecmev

Sounds like something @remcohaszing has ideas on!

wooorm avatar Aug 22 '22 10:08 wooorm

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 (https://github.com/davidbonnet/astring/pull/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 :exploding_head: 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 />;

remcohaszing avatar Aug 22 '22 15:08 remcohaszing

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: https://github.com/microsoft/TypeScript/pull/41330#issuecomment-719065708 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: https://github.com/microsoft/TypeScript/issues/47072#issuecomment-990619977 The JSX namespace is an implementation detail, not meant to be public. So, in the end, import { HResult } is the way to go.

alecmev avatar Aug 22 '22 16:08 alecmev

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

wooorm avatar Aug 23 '22 18:08 wooorm

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.

alecmev avatar Aug 23 '22 19:08 alecmev

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

wooorm avatar Aug 24 '22 07:08 wooorm

Hi! This was marked as ready to be worked on! Note that while this is ready to be worked on, nothing is said about priority: it may take a while for this to be solved.

Is this something you can and want to work on?

Team: please use the area/* (to describe the scope of the change), platform/* (if this is related to a specific one), and semver/* and type/* labels to annotate this. If this is first-timers friendly, add good first issue and if this could use help, add help wanted.

github-actions[bot] avatar Aug 24 '22 07:08 github-actions[bot]

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

github-actions[bot] avatar Oct 10 '22 18:10 github-actions[bot]

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 avatar Oct 10 '22 18:10 wooorm

Sorry for the radio silence, thank you!

alecmev avatar Dec 22 '22 15:12 alecmev