Add file extensions to TS imports #565

mhsdef · 2024-01-23T19:48:00Z

See: https://nodejs.org/docs/latest-v20.x/api/esm.html#mandatory-file-extensions and nodejs/node#46006

(And all the related issue malarkey linked off from there too!)

tl;dr: I was getting this at runtime with jssm in Node 20.x + TypeScript 5.3.3:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/app/node_modules/jssm/dist/es6/jssm_types' imported from /app/node_modules/jssm/dist/es6/jssm.js
    at finalizeResolution (node:internal/modules/esm/resolve:255:11)
    at moduleResolve (node:internal/modules/esm/resolve:908:10)
    at defaultResolve (node:internal/modules/esm/resolve:1121:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:396:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:365:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:240:38)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:85:39)
    at link (node:internal/modules/esm/module_job:84:36) {
code: 'ERR_MODULE_NOT_FOUND',
url: 'file:///app/node_modules/jssm/dist/es6/jssm_types'
}

Node.js v20.10.0

So I went hunting, debuggin', hacking and found the changes here seem to be the correct fix.

I've confirmed it fixed the import problems for me locally in my application.

But I'd appreciate if you could give this a run through in your tests, run the build stuff as needed if it looks good to you, and cut us a new release.

Great library, keep it up, hope that book is still coming! Cheers!

StoneCypher · 2024-01-24T04:49:49Z

i believe you and i want to help, but also, there are many loader paths in modern javascript, and so i need to know which one is broken

like, it works as expected in node require and node dynamic import

it works as expected in rollup and esbuild

it works as expected in es6 dynamic load with defer and not under CORS

i need to know how you're loading things so that i can see it go south

a repro would help a whole lot

mhsdef · 2024-01-24T04:53:12Z

fsm.ts:

import * as jssm from "jssm";

tsconfig.json:

{
	"$schema": "https://json.schemastore.org/tsconfig",
	"display": "Node LTS",
	"_version": "20.1.0",

	"compilerOptions": {
		"lib": ["es2023"],
		"module": "node16",
		"target": "es2022",

		"strict": true,
		"esModuleInterop": true,
		"skipLibCheck": true,
		"forceConsistentCasingInFileNames": true,
		"moduleResolution": "node16",
		"outDir": "dist",
	}
}

package.json part of interest:

	"engines": {
		"node": "20.x"
	},
	"type": "module",
	...
	"devDependencies": {
		"@biomejs/biome": "1.5.3",
		"@types/luxon": "^3.4.2",
		"lint-staged": "^15.2.0",
		"rimraf": "^5.0.5",
		"tsc-watch": "^6.0.4",
		"typescript": "^5.3.3",
		"vitest": "^1.2.1",
		"vitest-mock-extended": "^1.3.1"
	}

mhsdef · 2024-01-31T19:43:32Z

@StoneCypher any verdict here yet?

As far as I can tell, this approach is aligned with TS/Node best practices in 2023. So if we pass your tests ok...gtg?

kaxline · 2024-02-03T17:43:01Z

@StoneCypher Are you able to get it to build with tsc, ts-node, or ts-loader? I'm running into the same issue with a standard NextJS Typescript setup. Going to patch it with @mhsdef 's changes for now.

StoneCypher · 2024-02-05T19:59:58Z

to build with tsc, just tsc --config tsconfig.json, or npm run build

i've never tried with ts-node or ts-loader. i suppose i ought to

the typescript manual still explicitly says "don't do this," is why i'm dragging my heels

i'm starting to think what i should actually do for es6 is ship bundled and rely on external tree shaking. that obviates the discussion, because rollup knows what to do here

mhsdef · 2024-02-05T20:34:54Z

the typescript manual still explicitly says "don't do this," is why i'm dragging my heels

Forgive if it's on page one lol.... could you point us to that?

StoneCypher · 2024-02-05T22:42:23Z

this has been a neckbeard fight between the typescript and node tribes for like five years now and i kind of just don't want to get involved in it, is the honest truth, and i kind of just don't trust that it's over

also i tried this and my editor's ts hinting broke, and like. ugh

i apologize for the ultra-slow turnaround here. in the last 14 days we've had three record breaking storms for 8 days underwater in a theoretically desert city that is entirely unready for this kind of shit. i'm from back east, where this kind of rain wouldn't be a big deal, but out here it's washing cars away instead, which is kind of wild

the original reason that es6 imports pointed to an unbundled version of the lib was twofold. one, back then, it made a difference for tree shaking. now that's actually a tree shaking mistake. two, back then, it made in-browser debugging easier. but since then i started shipping source maps, which chrome can unformat with, so, ... whatever.

bundling here just puts the problem to bed mafia enforcer style, so, nite-nite it goes, i think.

i'm not going to close this until both @mhsdef and @kaxline confirm that this resolves their loader problem.

in the meantime, the fact that i was unaware of this is in itself kind of unacceptable, so i'm going to build out a loader matrix in the test set, which covers various node importations, browser, bun, deno, ts-node, ts-loader, et cetera

i hope this is okay, that i'm offering an alternative solution. i actually do generally take PRs gladly, because i want to see that contributor count go up

but right now i think i've been burned on the extension nonsense enough times, and i'm change-shy

i hope that isn't a downer

i pushed 5.97.0 just now. in theory it should fix both problems

i'll try to have a build target loader matrix together at the fundamentals by 5.102 or so, and done by maybe 5.110 or so. no promises

mhsdef · 2024-02-05T23:22:14Z

I'll have to dig in at it more later, but, early feedback, no dice:

StoneCypher · 2024-02-05T23:23:56Z

Oh, the .d.ts is pointing to the wrong file name

…

On Mon, Feb 5, 2024, 3:22 PM Hew ***@***.***> wrote: I'll have to dig in at it more later, but, early feedback, no dice: Screenshot.2024-02-05.at.6.21.07.PM.png (view on web) <https://github.com/StoneCypher/jssm/assets/2006764/bb0baf62-212e-4bba-89ce-0d8fbff6844c> — Reply to this email directly, view it on GitHub <#565 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAS5KWDZWQA25NDZBTLPHLYSFSTDAVCNFSM6AAAAABCHSMXA2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRYGQ4DGMJVHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

StoneCypher · 2024-02-05T23:58:16Z

this hot mess appears to be the solution

i am getting annoyed with ts right now

mhsdef · 2024-02-06T05:54:38Z

This seems to help:
https://stackoverflow.com/a/77566206

When I edit your package.json to this -- we seem closer:

	"exports": {
		"require": "./dist/jssm.es5.cjs",
		"import": {
			"types": "./jssm.d.ts", (or "./dist/es6/jssm.d.ts")
			"default": "./dist/jssm.es6.mjs"
		},
		"default": "./dist/jssm.es5.cjs",
		"browser": "dist/jssm.es5.iife.cjs"
	},

"Funnily", though, all this breaks some typing for me that works fine with my patch. Specifically, the data payload in callbacks. Eg:

	machine.hook_exit(IncidentState.Acknowledged, ({ data }) => {
		return isString(data.acknowledgedAt);
	});

It had been passing though fine via the generic in the machine instantiation, but, now it is untyped...

mhsdef · 2024-02-06T06:14:39Z

~~I think it may need a distinct .d.mts to be proper, tho it seems to be resolving the explicitly set .d.ts even though I see reports of mismatch there being problematic.~~ TS seems to resolve .d.ts ok with the explicit import: { "types" ..., "default": ... } define in package.json.

Also noticed "module": "dist/jssm.es6.js", -- I think the exports are superseding, but that should prob be dist/jssm.es6.mjs now ya?

mhsdef · 2024-02-06T14:51:43Z

"Funnily", though, all this breaks some typing for me that works fine with my patch. Specifically, the data payload in callbacks.

I'm pretty sure this is back to the file extension issue. HookHandler is over in jssm_types.d.ts which is imported with a bare identifier import. Fails, any. From your TS doc link above:

Relative import paths need full extensions (we have to write import "./foo.js" instead of import "./foo").

StoneCypher · 2024-02-08T17:10:33Z

i am, slowly, coming around to the position that changing this may in fact be correct

right now i'm kicking around a tool in my head which would validate the external interface of a library from [node|deno|bun|browser][context] so that this can be directly vetted

i don't want to put testing that directly in the library because, well, good god, installing or relying on several other languages just for a test set would be a miserable thing

however doing this without validation is rapidly becoming untenable

mhsdef · 2024-02-13T14:23:51Z

@StoneCypher just checking back, any verdict?

I'm going to have to ship imminently. I can fork and add my own version as dep I spose to buy more time.

kaxline · 2024-02-25T15:59:14Z

@StoneCypher @mhsdef Wow, really appreciate all the hard work here. Don't wait on me to approve anything if the two of you are able to get it working. Hopefully anyone can understand this is not a mess of your creation!

I've been battling for weeks at my day job over this ts-loader business so I feel the frustration as well. Sorry I can't dive in and help here.

Not sure it's relevant, but I've had some success using npx ts-node-esm to run node scripts written in Typescript and this library may offer some relief:

https://github.com/privatenumber/tsx

mhsdef · 2024-02-25T16:46:19Z

I have it in usage with a fork at this point.

It's 183e721 (this change) and mhsdef@c3510f4 (npm run typescript).

StoneCypher · 2024-02-28T14:58:28Z

I'm currently making a tool to test platforms without being part of the library using it

Once I have that tool, I can test merging this safely

StoneCypher · 2024-02-28T15:00:41Z

Honestly, I've always wanted having the extensions back. The disparity from the rest of the JS world has always felt gross (albeit I guess I've kind of gotten used to it)

What's making me so reluctant is there are umpty million quasi-typescript targets, like deno and bun and rome and so on

I'm worried that this is a short solution to the ones we're looking at but a long solution to those, and I'm heel-dragging because it still smells like there might be another option

Once I can stamp the whole set simultaneously without polluting the downstreams of users, I can stop staring off into the sunset trying to intuit what the right thing to do is, and just measure the damned thing

StoneCypher · 2024-05-23T00:02:31Z

a microsoft staff member just tried to commit something similar

the referenced threads have other microsoft staff members doubling and tripling down that this is verboten

i am very confused rn

jason-ha · 2024-05-23T01:24:13Z

Hey @StoneCypher, I was just trying to go through some of the issues here and I am wondering where the guidance is to not add extensions and use full paths for javascript imports. The Microsoft TypeScript team very much is opposition to use of .ts, but as far as I know they are in favor of .js. They want import paths to meet the resolution requirements of the environment it has been asked to transpile for and they will not do any rewriting of paths (won't change an extension for example).
The compilerOptions + package.json engine currently set in the project are behind minimum secure runtime environments. Maybe you need to support older situations using now unsupported versions of Node.js, but I would recommend raising the support bar to Node 18+. If you were to do that, then you would want to update compilerOptions module to Node16 and drop moduleResolution or at least set moduleResolution to Node16. The comments in tsconfig.json are slightly off. moduleResolution is not /* Specify how TypeScript looks up a file from a given module specifier. */ but really telling tsc what environment the produced code will be running in. So tsc will follow that declared environment's resolution process when doing its job because it does not want to rewrite import paths.
tsc is happy with .js extensions on relative paths and Node16 resolution because explicit paths are part of the Node16 resolution specification. Under Node10 resolution tsc will find the .js equivalent path from what it is given. So, tsc could look for /index.js in addition to trying to just add .js to a given path. Obviously in a typescript source environment there wouldn't be an actual .js file present; so, they would look for .ts and .d.ts files for those potential resolutions. Node16 does not have such ambiguity which makes the resolution so much simpler. Then with the .js equivalent resolved, tsc looks for .ts or .d.ts in that location.
If you were to change compilerOptions module to Node16 (and keep package.json type set to module, then tsc will enter a strict mode and will require that relative imports have .js extension. Try it out.
I did look briefly through some of this package's outputs and it looks complex. So, my suggested tsconfig change probably only applies to one case of output generation and is perhaps just informative to test out.

jason-ha · 2024-05-23T21:45:30Z

This seems to help: https://stackoverflow.com/a/77566206

When I edit your package.json to this -- we seem closer:
	"exports": {
		"require": "./dist/jssm.es5.cjs",
		"import": {
			"types": "./jssm.d.ts", (or "./dist/es6/jssm.d.ts")
			"default": "./dist/jssm.es6.mjs"
		},
		"default": "./dist/jssm.es5.cjs",
		"browser": "dist/jssm.es5.iife.cjs"
	},
"Funnily", though, all this breaks some typing for me that works fine with my patch. Specifically, the data payload in callbacks. Eg:
	machine.hook_exit(IncidentState.Acknowledged, ({ data }) => {
		return isString(data.acknowledgedAt);
	});
It had been passing though fine via the generic in the machine instantiation, but, now it is untyped...

There are two different issues going on here. One is the use of extensions of internal import paths related to Node16 based module resolution. The changes to the .ts files look good. But there aren't extensions added to the imports in .d.ts files. Extensions should also be updated to respect Node16 resolution. For the .d.ts import paths I think either .js or .d.ts would work.

The second issue is Node16 resolution from outside the package. In that situation all of the imports are resolved going through package.json exports specification. The lack of types condition and result meant no type information for any one compiling against jssm, but runtime would be fine. Then once fixed with tweak the types don't resolve because of the internal resolution issue - no extensions.

It is informative to run just tsc with declaration output and compare that to what is checked in.

tylerbutler · 2024-05-23T22:22:20Z

The second issue is Node16 resolution from outside the package. In that situation all of the imports are resolved going through package.json exports specification. The lack of types condition and result meant no type information for any one compiling against jssm, but runtime would be fine. Then once fixed with tweak the types don't resolve because of the internal resolution issue - no extensions.

I've updated the description in StoneCypher/fsl#1295 to clarify that my primary concern is @jason-ha's second issue - using jssm from a Node16 TypeScript library. I have proposed a change in the issue that does not require any runtime changes - only rollup config files, package.json, and adds two declaration rollup files that correspond to the bundled es5 and es6 JS. This sidesteps the internal resolution issue because there's no resolution needed to use the types - everything is in a single file.

mhsdef · 2024-05-23T22:56:17Z

The changes to the .ts files look good. But there aren't extensions added to the imports in .d.ts files.

@StoneCypher could chime in here authoritatively, but, I think that's solved via his TS build scripting -- npm run typescript. Or that's what I did anyways in operationalizing my fork. 😅

StoneCypher · 2024-05-23T23:55:29Z

yeah, that's one of the big problems here is that if i switch to .js, compat .ts has the wrong extension, and if i don't, then .d.ts points to the wrong file

i've thought about performing manual file surgery (like i do for the parser, admittedly) but it's just so gross

the current practice of naming .js on the out is typescript, but, i am not convinced of any particular configuration's correctness at this time

it's very messy

i believe that the current issue is getting node16 resolution to respect the .d.ts

i'm going to swear word with that for a while

if it turns out that changing the filenames is right i'll still land mhsdef's pr, but, that has been breaking my legacy builds and ends me up in effectively the same situation for just the other half of the output

StoneCypher · 2024-05-23T23:56:03Z

it's flattering to see this many people trying to collaborate to help me suck less at my hobby project

thank you each

Add file extensions to TS imports

183e721

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file extensions to TS imports #565

Add file extensions to TS imports #565

mhsdef commented Jan 23, 2024 •

edited

StoneCypher commented Jan 24, 2024

mhsdef commented Jan 24, 2024 •

edited

mhsdef commented Jan 31, 2024

kaxline commented Feb 3, 2024

StoneCypher commented Feb 5, 2024

mhsdef commented Feb 5, 2024

StoneCypher commented Feb 5, 2024

mhsdef commented Feb 5, 2024

StoneCypher commented Feb 5, 2024 via email

StoneCypher commented Feb 5, 2024

mhsdef commented Feb 6, 2024 •

edited

mhsdef commented Feb 6, 2024 •

edited

mhsdef commented Feb 6, 2024 •

edited

StoneCypher commented Feb 8, 2024

mhsdef commented Feb 13, 2024

kaxline commented Feb 25, 2024

mhsdef commented Feb 25, 2024

StoneCypher commented Feb 28, 2024

StoneCypher commented Feb 28, 2024

StoneCypher commented May 23, 2024

jason-ha commented May 23, 2024

jason-ha commented May 23, 2024

tylerbutler commented May 23, 2024

mhsdef commented May 23, 2024

StoneCypher commented May 23, 2024

StoneCypher commented May 23, 2024

Add file extensions to TS imports #565

Are you sure you want to change the base?

Add file extensions to TS imports #565

Conversation

mhsdef commented Jan 23, 2024 • edited

StoneCypher commented Jan 24, 2024

mhsdef commented Jan 24, 2024 • edited

mhsdef commented Jan 31, 2024

kaxline commented Feb 3, 2024

StoneCypher commented Feb 5, 2024

mhsdef commented Feb 5, 2024

StoneCypher commented Feb 5, 2024

mhsdef commented Feb 5, 2024

StoneCypher commented Feb 5, 2024 via email

StoneCypher commented Feb 5, 2024

mhsdef commented Feb 6, 2024 • edited

mhsdef commented Feb 6, 2024 • edited

mhsdef commented Feb 6, 2024 • edited

StoneCypher commented Feb 8, 2024

mhsdef commented Feb 13, 2024

kaxline commented Feb 25, 2024

mhsdef commented Feb 25, 2024

StoneCypher commented Feb 28, 2024

StoneCypher commented Feb 28, 2024

StoneCypher commented May 23, 2024

jason-ha commented May 23, 2024

jason-ha commented May 23, 2024

tylerbutler commented May 23, 2024

mhsdef commented May 23, 2024

StoneCypher commented May 23, 2024

StoneCypher commented May 23, 2024

mhsdef commented Jan 23, 2024 •

edited

mhsdef commented Jan 24, 2024 •

edited

mhsdef commented Feb 6, 2024 •

edited

mhsdef commented Feb 6, 2024 •

edited

mhsdef commented Feb 6, 2024 •

edited