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

Consuming projects can't find JSX.IntrinsicElements after v2.1.2 #38

Closed
spiffytech opened this issue Jan 24, 2022 · 13 comments · Fixed by #41
Closed

Consuming projects can't find JSX.IntrinsicElements after v2.1.2 #38

spiffytech opened this issue Jan 24, 2022 · 13 comments · Fixed by #41

Comments

@spiffytech
Copy link
Member

In v2.1.2 consuming projects can't find the JSX.IntrinsicElements interface. forgo is exporting JSX, but forgo.createElement isn't detecting that.

This was introduced by the merge of 812177f (my commit, my bad). I'm not sure why this didn't turn up when I tested of that commit.

I'll have a PR up once I identify a fix.

Prior to v2.1.2, forgo declared namespace createElement with namespace JSX inside of it. 812177f switched the full module import (import "./jsxTypes") to a types import (import type "./jsxTypes") to fix an esbuild issue, and that means the old way of declaring the createElement namespace doesn't work.

I'm trying to determine a new way to do that, and also why that's necessary in the first place. I can't find any documentation indicating TS JSX needs a createElement namespace. So presumably there's another way to go about the problem.

@jeswin
Copy link
Member

jeswin commented Jan 24, 2022

Interesting. When I tested with npx create-forgo-app my-project --template typescript I get full typing.

Perhaps I'm missing something?

@spiffytech
Copy link
Member Author

When I was creating a sandbox for the other issue (example) I noticed that upgrading to v2.1.2 caused JSX tags to have the error JSX element implicitly has type 'any' because no interface 'JSX.IntrinsicElements' exists. ts(7026). Downgrading to v2.1.1 removes the error. I pulled down the .zip of that sandbox and get the same error locally.

Interestingly, I don't get this problem with my application. So the difference may be related to the specific setup the code sandbox uses. But I want to investigate since this looks like a regression against a previously-working setup.

@jeswin
Copy link
Member

jeswin commented Jan 24, 2022

I agree. Especially since CodeSandbox is a very common usecase.

@spiffytech
Copy link
Member Author

spiffytech commented Jan 24, 2022

Looks like the difference between codesandbox and the CFA template & my own project is the latter have a tsconfig.json like:

    "jsxFactory": "forgo.createElement",
    "jsxFragmentFactory": "forgo.Fragment",

while the codesandbox template uses

    "jsxFactory": "createElement",
    "jsxFragmentFactory": "Fragment",

So not specific to the codesandbox environment, just an app config difference. If I reconfigure the codesandbox to use the former config style, the error goes away. I'll try to fix the codesandbox-style config.

I think the JSX namespace might need to be global, but I need to figure out how to convince TS to find Forgo's JSX namespace when we're only importing types from jsxTypes.ts.

@spiffytech
Copy link
Member Author

...which is probably exactly what the createElement namespace addressed. So I'll get that namespace working with the import type syntax and add a comment explaining why it's necessary.

@jeswin
Copy link
Member

jeswin commented Jan 25, 2022

You're right, sorry I should have remembered this. IIRC it was a lot of trial and error to get it working, but I had completely forgotten why and how it was done.

@spiffytech
Copy link
Member Author

I spent some tore time trying to piece together a solution today. Definitely trial and error.

TS doesn't like importing and re-exporting a createElement namespace when there's already a createElement function. And it doesn't like defining the namespace is index.js because namespaces don't play nice with import type.

I'll probably wind up switching back to an actual import and getting the build to include a module that can be imported, just to get the old way of handling this to work when run through esbuild.

@spiffytech
Copy link
Member Author

I've got the types issue sorted out, but I had to adjust the build pipeline and broke the source map in the process.

The import type solution is a dead end. If the namespace createElement is declared in jsxTypes.ts, there's no way to import and reexport it in index.ts without getting errors because it collides with the exported createElement() function. And if index.ts imports the JSXTypes namespace with import type, TS won't let you export it in a namespace created in index.ts. It's gotta be a plain ol' import.

(I also tried getting jsxTypes to be an ambient declaration with global scope, which works fine inside the forgo module, but consuming projects don't pick it up because it's not referenced in index.d.ts, and I couldn't find a way around that.)

So I walked the issue back: prior to enabling the minified build, a normal import worked when Forgo used the tsc output. So why not with esbuild? Because tsc understands the types and intelligently strips the imports that only deal with types, so they don't wind up in the final output. esbuild doesn't understand types -- just syntax -- and so it leaves the import ./jsxTypes in. Which breaks consuming projects because jsxTypes isn't actually a module since that file doesn't export anything but types. And if I export something just for the sake of it, the build complains that jsxTypes.ts doesn't export JSXTypes. So convincing esbuild to do the right thing with import seems like a dead end too.

The solution looks like running tsc and then pointing esbuild at the tsc output instead of the original source code. That gets us the namespaces, stripping the normal-style import, and letting esbuild do minification.

But that means we have to generate the source map from tsc, then get esbuild to process that when building the source map for the minified output. And so far I don't think that's working.

I've got a WIP branch pushed, and once I get the source map fixed I'll clean everything up and squash it down to one nice, tidy commit.

@spiffytech
Copy link
Member Author

It looks like esbuild is supposed to identify and drop type-only imports. But it doesn't recognize that this is a type-only import because without --bundle esbuild evaluates each file individually, so it doesn't know that the JSX imports are types at the time it processes index.ts. So I'm seeing if there's a way to correct that.

spiffytech added a commit that referenced this issue Jan 29, 2022
# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch and 'origin/fix-jsx-types' have diverged,
# and have 6 and 7 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch and 'origin/fix-jsx-types' have diverged,
# and have 6 and 7 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch and 'origin/fix-jsx-types' have diverged,
# and have 6 and 7 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#
spiffytech added a commit that referenced this issue Jan 29, 2022
# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch and 'origin/fix-jsx-types' have diverged,
# and have 6 and 7 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch and 'origin/fix-jsx-types' have diverged,
# and have 6 and 7 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch and 'origin/fix-jsx-types' have diverged,
# and have 6 and 7 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch is up to date with 'origin/fix-jsx-types'.
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#
spiffytech added a commit that referenced this issue Jan 29, 2022
# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch and 'origin/fix-jsx-types' have diverged,
# and have 6 and 7 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch and 'origin/fix-jsx-types' have diverged,
# and have 6 and 7 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch and 'origin/fix-jsx-types' have diverged,
# and have 6 and 7 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch is up to date with 'origin/fix-jsx-types'.
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Wed Jan 26 19:11:24 2022 -0500
#
# On branch fix-jsx-types
# Your branch is up to date with 'origin/fix-jsx-types'.
#
# Changes to be committed:
#	modified:   package.json
#	modified:   src/index.ts
#	modified:   src/jsxTypes.ts
#	modified:   tsconfig.json
#
@spiffytech
Copy link
Member Author

Okay, all set - PR is up.

The secret sauce is that JSX needs to be a namespace, but I guess import * from ... automatically becomes a namespace in the importing context, so jsxTypes.ts doesn't need to declare a namespace, which simplifies importing + reexporting that in a way that lets esbuild optimize out the types-only import.

esbuild also had to be put into bundle mode to detect that the import was types-only, and now also needs the --format=esm flag since bundle mode by default transforms the module into an IIFE.

I also simplified the way the bundle + types are declared in package.json.

Please give this a whirl and let me know if you find any problems. Or don't -- I may throw TypeScript into a volcano if this somehow still isn't fixed 🙂

@jeswin
Copy link
Member

jeswin commented Feb 2, 2022

I may throw TypeScript into a volcano if this somehow still isn't fixed

Haha. I'll try this today, and publish a new version. Thanks.

jeswin added a commit that referenced this issue Feb 13, 2022
…ory (#41)

Co-authored-by: spiffytech <git@spiffy.tech>
Co-authored-by: Jeswin <jeswinpk@agilehead.com>
@jeswin
Copy link
Member

jeswin commented Feb 13, 2022

@spiffytech Tested on CodeSandbox. https://codesandbox.io/s/forgo-todos-typescript-9v0iy

It works! 🥳

@spiffytech
Copy link
Member Author

LGTM!

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 a pull request may close this issue.

2 participants