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

[Feature] Add ability to output explicit indexes #136

Open
jacobbogers opened this issue Sep 17, 2021 · 29 comments
Open

[Feature] Add ability to output explicit indexes #136

jacobbogers opened this issue Sep 17, 2021 · 29 comments

Comments

@jacobbogers
Copy link

Hello,

Example: this (transpiled) code loads the index.js file

import  {  some_function } from '../siblingdir';   // there is an index.js in the "siblingdir"

If i try to use this in an es6 fashion I will get ERR_UNSUPPORTED_DIR_IMPORT Because it is not possible in the esm standard to do directory imports, it has to be fully specified import { some_function } from '../siblingdir/index.js

I wonder if there is an options where it detects:

  • if it is a directory, add an /index.js suffix
  • if it is a normal file add the explicit extension '.js'

Cheers,

@jacobbogers
Copy link
Author

https://nodejs.org/api/esm.html#esm_mandatory_file_extensions

Mandatory file extensions

A file extension must be provided when using the import keyword to resolve relative or absolute specifiers. Directory indexes (e.g. './startup/index.js') must also be fully specified.

This behavior matches how import behaves in browser environments, assuming a typically configured server.

@nonara
Copy link
Collaborator

nonara commented Sep 18, 2021

No option at the moment. Would be pretty easy to implement, though. I'd be open to a PR as a useExplicitIndexes option.

I was also considering adding useExplicitExtensions. Another esm reconciliation feature.

Like I said, PR welcome! Otherwise I may add it when I have some time.

@jacobbogers
Copy link
Author

I found a "how to build an plugin" doc, I am reading it

@nonara
Copy link
Collaborator

nonara commented Sep 18, 2021

Mandatory file extensions

Yep. That's the reason for the second option. Esm is a pain 😄

@nonara nonara changed the title ERR_UNSUPPORTED_DIR_IMPORT for directory import [Feature] Add ability to output explicit indexes Sep 18, 2021
@nonara
Copy link
Collaborator

nonara commented Sep 18, 2021

Eh. It should be pretty easy on both fronts, and there don't seem to be any good solutions out there for this in esm.

I'll try to knock these out tomorrow.

@jacobbogers
Copy link
Author

How is it going?

@nonara
Copy link
Collaborator

nonara commented Sep 18, 2021

Finished. Just working on integrating with tests. I opted for adding a config property: outputMode which is either commonjs | esm. When not specified, it's commonjs by default.

Behaviour is to add explicit extension and index if in esm mode. And, for declaration file output, outputMode will always set itself to commonjs, as I do not believe .d.ts has an esm-specific difference. Do you know if that's correct?

@jacobbogers
Copy link
Author

Super thank you,

About your question:

I do not believe .d.ts has an esm-specific difference. Do you know if that's correct?

Honestly I do not know this, I dont think *.d.ts file have any knowledge about esm or commonjs

What about the *.d.ts.map files? Are they generated after all "transpilations" runs? (I think so, but I ask just to make sure)

@nonara
Copy link
Collaborator

nonara commented Sep 18, 2021

No problem!

Documentations indicates that TS uses their own "ES-like" syntax for modules, so I think we're covered. As for map files, those should be generated post-transform.

Waiting 'til tomorrow on another PR to do an official release, but I've published an alpha. Would you mind checking if it works for you?

{
  "devDependencies": {
    "typescript-transform-paths": "3.4.0-alpha"
  }
}

@jacobbogers
Copy link
Author

Would you mind checking if it works for you?

Yes,I am checking it right now

@jacobbogers
Copy link
Author

jacobbogers commented Sep 18, 2021

Maybe I am doing something wrong, but It did not work for me

In my node_modules I have installed the correct verison

{
  "name": "typescript-transform-paths",
  "version": "3.4.0-alpha",

My tsconfig.json looks like so:

"compilerOptions: {
      "plugins": [
            {
              "name": "typescript-eslint-language-service"
            },
            {
              "transform": "typescript-transform-paths",
              "useExplicitIndexes": true,
              "useExplicitExtensions": true
            },
            {
              "transform": "typescript-transform-paths",
              "afterDeclarations": true
            }
}

But the transpiled code not showing explicit .js or /index.js additions in the path

import { fixup } from '../fixup';   // no .js extension
import { IRNG } from '../irng';
import { IRNGTypeEnum } from '../irng-type';
.
.

Maybe I am configuring it incorrectly, I do not know for sure.

@nonara
Copy link
Collaborator

nonara commented Sep 18, 2021

Sorry, I should have specified. "outputMode": "esm" not useExplicitIndexes or useExplicitExtensions

@jacobbogers
Copy link
Author

ok Checking with your parameters

@jacobbogers
Copy link
Author

Works, shows explicit /index.js at the end of a directory import

'use strict';
export * from "./distributions/beta/index.js";
export * from "./distributions/binomial/index.js";
export * from "./distributions/binomial-negative/index.js";
export * from "./distributions/cauchy/index.js";

and also .js for individual regular files

export { dbinom } from "./dbinom.js";
export { pbinom } from "./pbinom.js";
export { qbinom } from "./qbinom.js";
import { rbinomOne } from "./rbinom.js";
import { globalUni } from "../../rng/global-rng.js";
import { repeatedCall64 } from "../../r-func.js";

Super thank you!! Now I can release esm version

@jacobbogers
Copy link
Author

Wait, something not completely correct...

@jacobbogers
Copy link
Author

Ok, I found one issue,

I am importing the debug module link here

The original typescript looks like so:

import { debug } from 'debug';
import { isOdd } from '@lib/r-func';

Transpiles it looks like so:

import { debug } from "../../../node_modules/@types/debug";
import { isOdd } from "../r-func.js";

Seems it goes to node_modules/@types but not to node_modules/debug

@nonara
Copy link
Collaborator

nonara commented Sep 19, 2021

Is this repo public so I can take a look?

@jacobbogers
Copy link
Author

yes it is public, let me clean it up for you, i was writing jest tests))))

@jacobbogers
Copy link
Author

I set outputMode to "esm" in the main of this repo https://github.com/jacobbogers/libRmath.js.git

npm i
npm run build 

It builds for node in <rootDir>/es6 and for the browser in <rootDir>/browser

The latter fails because of the issue.

Thanks for checking

@nonara
Copy link
Collaborator

nonara commented Sep 19, 2021

Thanks. I uncovered a few more complicated gotchas that could arise. ES with TS is a bit of a nightmare.

Will be wrapping this up tomorrow. I'll let you know when there is a new beta.

@nonara
Copy link
Collaborator

nonara commented Sep 20, 2021

Quick update on this —

In order to make this work I had to take down a fence that was keeping out a lot of nasty bugs. Namely, we were only transforming if a path matched or if there was an explicit tag. In this case, however, we're going to have to handle any and all paths if esm mode is set.

This means we have to intelligently handle external package import (et al.) without transforming the path to the relative path. (ie. package/file should be package/file.js not ../node_modules/package/file.js).

We need to do some special handling to determine what's path mapping and what isn't, as well as differentiating between an external package import vs a sub-package import, which gets even more tricky with path mapping.

In addition to that, it occurred to me that we're going to have to support ESM's weird import structure, where file paths don't necessarily correspond to the actual resolved file path.

Ie. @scope/my-package/path/to/file.js can point to dist/somefile.js in the package.

Or in a non-external package, with mapping, #mapped/to/file.js where config is: #mapped/*: [ "./my-package/path/*" ]

Bottom line is, these things are important, as full esm support should be the goal. Because of the scope of the changes, I'll be releasing this as a new major, and will therefore try to also incorporate some performance enhancement and the other esm features in the pipeline in the same release. I actually might add a middleware option also to allow people to hook the resolution process.

The good news is, I've just about finished the resolution rewrite which handles all of the aforementioned and other issues. But, it's going to need a comprehensive test suite to ensure all the gotchas are covered, so it'll take some more time.

I'll get back at it next weekend. The goal is for it to be wrapped by the end of the month.

@jacobbogers
Copy link
Author

@nonara Super thanks for all your efforts, I think you are actually the first one to solve the esm path issue so extensively.

@jacobbogers
Copy link
Author

How is it going @nonara, is there a branch where you are working on this, I may be able to help out starting from next week

@nonara
Copy link
Collaborator

nonara commented Oct 11, 2021

Hi, Jacob! It's going well. I actually went full-time on this library for awhile now in order to get everything done. Lots of really cool stuff coming in the next major release. My PR to ts-node was also just merged, so the new esm loader will be included as well.

Everything is done, but I need to add some tests for the new custom test framework I built. I also need to add tests for the resolver middleware and create some example projects for the docs.

Also (not sure if you saw), but the TS 4.5 beta that just came out has implemented some major changes which specifically center around esm and module resolution. I think we're covered for everything, but I still need to check to see if there were any changes in their compiler API (factory functions, resolver, etc). So, the last item on my TODO is to test with that and integrate any changes necessary to ensure it's compatible with that as well.

Here's a look at the new settings:

export interface TransformerConfig {
  readonly useRootDirs?: boolean;
  readonly exclude?: string[];

  // New options below:
  readonly usePaths: boolean;
  readonly outputIndexes: 'auto' | 'always' | 'never'
  readonly outputExtensions: 'auto' | 'always' | 'never'
  readonly resolver?: string;
}

The good news is, we're almost there. I'm out for this week, but I will likely be wrapping things up this coming weekend. If there's any opportunity to delegate some pieces, I'll reach out. Thank you for the offer!

@nonara
Copy link
Collaborator

nonara commented Oct 31, 2021

Hey Jacob! I know you've been waiting awhile on this, so I decided I'd go ahead and take my v4 branch live. That way you can clone the repo, check out my branch, build it, and use npm to create a zipped package to install.

I believe everything should work for your purposes. There are still a few edge cases, but probably unlikely to affect you. Go ahead and give it a shot, and if you hit any issues, let me know. If you do, I'll see if I can get a quick fix for you when I find a minute, or you can have a look, too, if you like.

You'll want to set

{
  "outputIndexes": "always",
  "outputExtensions": "always"
}

(Note: It does requires TS >= 4.2.2, though)

The branch is named: v4

If you're curious, what's left is:

  • Create tests for new test framework (Underscore Test)
  • Create tests for custom resolver
  • Test usePaths option
  • Add new esm loader
  • Add jest transformer wrapper
  • Ensure TS 4.5-beta is supported
  • Rewrite readme and add example files

Got a ways to go, but getting there! Unfortunately, I haven't had much time to work on it lately. Hopefully in a few weeks, I'll be able to wrap it up.

@jacobbogers
Copy link
Author

Hi @nonara
What very good news, I am almost finished with lib-r-math.js 2.0.0-alpha (only one distribution left to test) going to give your branch a look, and use it extensively,
I have time this week to help out if you want, thank you for all your efforts

@nonara
Copy link
Collaborator

nonara commented Nov 1, 2021

Cool! That sounds good. I appreciate it. If you familiarize yourself with the codebase and feel confident on it, you can look at the current failing tests. There's an edge case where it's not including the full subdir path for package main implicit index.

No rush though! It'd be good to have someone else who had a handle on how everything is laid out. I'm really happy with the new structure + test lib!

@SammyWhamy
Copy link

How's the progress on releasing this? This is the exact issue i've been running into for a while and this seems like the perfect fix!

@nonara
Copy link
Collaborator

nonara commented Oct 20, 2022

@SammyWhamy I launched a development firm over the last year and have been very tied down. I'm working on a plan to get this finished! It's nearly there, but there are still some edge cases, and I need to build tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants