tsdx icon indicating copy to clipboard operation
tsdx copied to clipboard

Modules resolved absolutely against `src` are treated as external: support TS Paths aliases

Open a-type opened this issue 6 years ago • 34 comments
trafficstars

Followup to #89 and #87.

Current Behavior

Running tsdx create initializes a tsconfig.json with absolute module resolution relative to the src directory. I have VS Code configured to prefer absolute paths if possible when importing modules, so if I have a module like...

src/foo.ts

export const foo = () => 'bar';

And I type

foo()

and hit Tab, VS Code will add the following import:

import { foo } from 'foo';

This is valid and properly resolved by Typescript. However, if I compile my library, the current Rollup behavior assumes that foo is an external module and doesn't include it in the bundle.

Expected behavior

I would expect my local modules to be included in my bundle regardless of whether I imported them using 'absolute' resolution as above,

OR

I would expect tsdx to not initialize tsconfig.json with resolution rules which would lead me to believe importing modules relative to src is a valid thing to do.

Suggested solution(s)

Either fix the external modules rules to properly understand local src-based resolution, or remove the default src resolution rules from tsconfig.json to avoid confusing users.

Additional context

I probably hit this more often than most because I have configured my editor to prefer absolute path resolutions if available.

Your environment

Software Version(s)
TSDX 0.5.7
TypeScript 3.4.3
Browser N/A
npm/Yarn NPM 6.2.0
Operating System Windows 10

a-type avatar May 08 '19 21:05 a-type

great issue. we should probably resolve this on a Rollup level. thanks for writing it up. i'll take a look at the rollup config but am not sure how to solve it

swyxio avatar May 08 '19 22:05 swyxio

options:

swyxio avatar May 08 '19 22:05 swyxio

probably want to add it here. want to try it in a local version and PR?

swyxio avatar May 08 '19 22:05 swyxio

I defer to you @sw-yx

jaredpalmer avatar May 08 '19 22:05 jaredpalmer

Continuing down this rabbit-hole: The potential issue I see with aliasing is that the tsconfig is bootstrapped into the user's controlled files, but the rollup config is still controlled by tsdx. If the src relative resolution is hard-coded into tsdx's rollup config, you may encounter a scenario where the user has a legitimate reason to remove the resolution from tsconfig.json, but cannot also remove it from Rollup. For instance, if there was a name conflict with an NPM module and they decided to ditch the src resolution.

So it would seem that we might want to detect if the user has modified their module resolution rules in tsconfig.json, or prevent them from doing so, or at least warn them of the behavior if they do.

It's kind of a headache that TS module resolution is decoupled from the actual module resolution used to build the bundle, and the two are split between user control and tsdx internals. It's confusing even in normal TS development... if only the rules set in tsconfig.json could also just automagically control how a final bundle is assembled.

a-type avatar May 09 '19 14:05 a-type

we could handroll (or publish) a plugin to read tsconfig.json... the source doesnt look too bad? basic or we could fork the alias plugin @a-type

swyxio avatar May 09 '19 14:05 swyxio

Please keep the current behaviour.

I have muliple small tsdx packages inside a mono repository which look like this (names simplified):

├── core
└── utils
    ├── util1
    ├── util2
    └── util3

Now when published to npm it is important that util1 does not inline core but uses it as an external.

So it is really important for me that import { core } from 'core'; is used as an external although the files are not coming from a node_modules folder.

jantimon avatar May 12 '19 09:05 jantimon

@jantimon doesnt that just mean core should be marked as a peer dependency? we can make sure to respect that too.

swyxio avatar May 12 '19 15:05 swyxio

Yes core is a peer dependency

jantimon avatar May 12 '19 15:05 jantimon

Should we make an external flag where you can hand write externals?

jaredpalmer avatar May 16 '19 22:05 jaredpalmer

I'd like to add to this, that using absolute paths for imports is causing resolution issues in my generated declaration files. My index.ts looks like this:

import * as Auth from "./auth";
import * as Request from "./common/api/request";
import * as Errors from "./common/api/error";
import { QueryParams } from "./common/api/query";
import { Settings } from "./common/api/settings";
import * as Structs from "./common/structs";
import * as Notifications from "./notifications";
import * as Account from "./resources/accounts";
import * as ChangeLog from "./resources/changelog";
import * as Hubs from "./resources/hubs";
import * as Billing from "./resources/billing";
import * as Containers from "./resources/containers";
import * as Infrastructure from "./resources/infrastructure";
import * as Stacks from "./resources/stacks";
import * as Images from "./resources/images";
import * as DNS from "./resources/dns";
import * as Environments from "./resources/environments";
import * as Jobs from "./resources/jobs";
import * as Secrets from "./resources/secrets";
import * as Projects from "./resources/projects";
export { Capability } from "./resources/hubs";
export { Auth, ChangeLog, Request, Errors, QueryParams, Settings, Structs, Account, Hubs, Billing, Containers, Infrastructure, Stacks, Images, DNS, Environments, Jobs, Secrets, Projects, Notifications, };

and in my declaration files I'm getting stuff like:

image

mattoni avatar May 30 '19 04:05 mattoni

Just checking in to see if there is a solution to the absolute pathing that I'm missing, or if this issue is truly related. Would love to use tsdx, it's extremely valuable.

mattoni avatar Jun 20 '19 19:06 mattoni

Not sure if the below issue is related to this thread or not: Rollup changes import of absolute path to relative path while bundling

I am importing a absolute path in some files of my repo and try to bundle it with rollup My import where /api/ is absolute path: import * from '/api/myFile.js'

But when I bundle it, rollup changes it to relative path and it looks like: import * from from '../../../../api/myFile.js'

And the above path doesn't exist in my app :(

My rollup config: rollup src\input.js -o lib\bundle.js -f esm --inlineDynamicImports=true

I tried making '/api/' path as external, that didn't change anything. And i tried using few rollup plugins - includepaths, root-import,etc nothing worked Is there any solution to this ?

RJahnavi avatar Jul 16 '19 18:07 RJahnavi

no idea tbh. not spending any time on this right now. i'll return to this once i'm in build mode. happy to take a look at PRs

swyxio avatar Jul 17 '19 07:07 swyxio

@sw-yx @jaredpalmer any examples on a mono repo with tsdx setup?

export-mike avatar Aug 29 '19 14:08 export-mike

no. check the other issues for other mono repo folks. we can maybe create an umbrella issue to look holistically at monorepo issues and appoint a maintainer to look after that bc i dont think either jared or i spend that much time in monorepo land.

swyxio avatar Aug 29 '19 15:08 swyxio

Not sure if this is a separate issue, or the same problem: Absolute imports does not work. Repo to reproduce: https://github.com/arvigeus/tsdx-broken-lib Tried the suggested solutions, didn't worked

arvigeus avatar Dec 11 '19 19:12 arvigeus

yeah there've been a few issues on this too. havent spent much time on it. sidenote tho - monorepo support is about to get a lot better since jared moved formik to monorepo

swyxio avatar Dec 12 '19 17:12 swyxio

Though I've gotten the relative paths to build, the typings are all messed up so I'm getting "any" for any type that's imported absolutely when trying to use the lib in production.

The definition file that was generated by tsdx (after configuring babel per the HOWTO guide on setting up absolute imports)

image

and attempting to use the getRequest() function which relies on a definition for an absolute import:

image

mattoni avatar Feb 11 '20 23:02 mattoni

I've wasted a day to make import absolute paths work. If you reading this I hope you don't make the same mistakes I did. No an expert but I got it working as follow:

-- Just an example --

// Say this is a React UI lib
./src
	/components/button.tsx
  	/components/..
  	/index.tsx // export {default as Button} from './components/button';
    /utils/math.tsx
    /utils/string.tsx
./test
  	/button.test.tsx
// tsconfig.json
{
  // ..
  "compilerOptions" : {
   // If you edit here you ALSO have to edit `.babelrc.js` and `jest.config.js`
   "paths": {
     // "*": ["src/*", "node_modules/*"] // "*": "src/*" causes much trouble than it tries to solve.
     "*": ["node_modules/*"] 
     "#comps": ["src/*"],
     "#utils/*": ['src/utils/*'],
    },
  }
}
//.babelrc.js
module.exports = {
  // ..
  "plugins": [
    [
	  // npm i -D babel-plugin-module-resolver
      // Alias resolver for `tsconfig.paths`
      // If you edit `tsconfig.json` paths you have to edit also here.
      "module-resolver",
      {
        "extensions": ['.ts', '.tsx'], // not sure if you really need this
        "root": "./",
        "alias": {
          // similar declaration as tsconfig.json but it's actually different !!
          "#comps": "./src", // note that './' is important here.
          '#utils': ['./src/utils'],
          
          // "*": ["./src/*"] 
          // "#utils/*": ['./src/utils/*'],
          // ☝️ Any path with "*" won't work as you expect for module-resolver 
          // since it treats as a character instead of a glob pattern. 
          // Wasted a day because of this stupid mistake.
        }
      }
    ]
  ]
}
// jest.config.js
module.exports = {
  //..

  // Alias resolver for `tsconfig.paths` in Jest
  // If you edit `tsconfig.json` paths you also have to edit here.
  moduleNameMapper: {
    // https://kulshekhar.github.io/ts-jest/user/config/#paths-mapping
    '^#comps$': '<rootDir>/src',
    '^#utils/(.*)$': '<rootDir>/src/utils/$1',
  },
}
// > src/tests/button.test.tsx <
// works fine with vscode and when you run `npm test`
import {Button} from '#comps';
import math from '#utils/math';
// ..

// > src/some/deep/dir <
// works fine with vscode and when you run `npm start/run build`
import {Button} from '#comps';
import math from '#utils/math';
// ..

The comment https://github.com/jaredpalmer/tsdx/issues/379#issuecomment-568202209 (require('./tsconfig.json').compilerOptions.paths) made me confuse how babel-plugin-module-resolver was suppose to work.

I still have to test with storybook and make sure everything works.

zprjk avatar Mar 08 '20 03:03 zprjk

I'm interested in getting a solution for this getting shipped into TSDX.

  1. Is there alignment here on how exactly this should work as a first class TSDX feature?
  2. Is there currently work being done to get this added to TSDX itself? It seems like a lot of people are dropping solutions they've made by extending the base TSDX config, but is anyone planning to work on a PR?

chrbala avatar Mar 12 '20 01:03 chrbala

  1. Is there alignment here on how exactly this should work as a first class TSDX feature?

Not exactly. The TS team itself isn't entirely supportive of paths being used like this or of TS transforms. Basically paths was built to help resolve type information on platforms that resolve differently, but it has grown to be used completely differently from intended (the reverse usage basically) as far as I understand. Can read my summary of the caveats at https://github.com/jaredpalmer/tsdx/pull/374#issuecomment-569369106

There is some notion of using loaders to resolve this and I think that's probably optimal. CRA has had some issues around this too and hasn't quite solved this or baseUrl stuff either. I think the confusion around the TS's team's response is also causing confusion on what to do.

Users clearly want this quite a lot, in TSDX and TS more broadly, so IMO, I think we should support this usage.

2. Is there currently work being done to get this added to TSDX itself? It seems like a lot of people are dropping solutions they've made by extending the base TSDX config, but is anyone planning to work on a PR?

Aside from the dropped #374, no. It's on my personal roadmap though because of significant popularity. I think out-of-the-box support for this should do something similar to https://github.com/jaredpalmer/tsdx/issues/379#issuecomment-568202209 to re-use paths for resolving modules during builds and tests. Re-using paths would be ideal so that there is a single source of truth.

That HOWTO doesn't quite work as @zprjk test's have revealed (sorry you had to debug all that, I recently learned about the globbing differences too while researching this) so some modifications would need to be made.

tsdx test doesn't currently use the same default babel plugins that tsdx build does, which is also on my roadmap and somewhat of a prerequisite (though can be worked around if someone wants to make a PR faster), see also #383 .

I'm also writing up an RFC for TSDX plugins (EDIT see #635) that could externalize this, but given its integration into tsconfig.json, tsdx build, and tsdx test, it probably makes more sense as internal

agilgur5 avatar Mar 12 '20 03:03 agilgur5

That's a really great synopsis of the situation - thank you! Given all of that, over the coming weeks, I may do some research into this and will report findings back here. It's not directly on our roadmap at the moment, so it may not be super timely, but we can make a bit of room for this.

chrbala avatar Mar 12 '20 04:03 chrbala

I guess this would rather be an anti-pattern for library development as users of the library might have to adjust their tsconfig to use the library sources

jantimon avatar Mar 17 '20 11:03 jantimon

@jantimon you're correct if there were no additional transforms -- but a portion of this issue and the surrounding ones are around adding such a transform so that one can use paths aliases beyond their original intent (which much of the TS community wants to be able to do, per my above links)

agilgur5 avatar Mar 17 '20 17:03 agilgur5

Workaround

To stop vscode using absolute imports when it auto imports, and thus my builds being broken, I was able to add the following to my workspace file:

"settings": {
    "typescript.preferences.importModuleSpecifier": "relative"
  }

chmac avatar May 28 '20 13:05 chmac

I've worked on a solution that uses absolute paths using @/ with tsdx, and made sure that these modules are not treated as external in tsdx-absolute-paths. If you're interested in making this an out-of-the-box feature with tsdx, what would be the next steps to get this shipped?

vkondamu avatar May 29 '20 19:05 vkondamu

tscpaths can also be used to change the absolute paths to relative paths for your build.

makker avatar Jun 11 '20 18:06 makker

Any updates on this one?

pedroapfilho avatar Aug 19 '20 21:08 pedroapfilho

@pedroapfilho the buggy default configuration has been removed from the templates as of v0.13.3, which resolves this issue's "OR" expected behavior.

I've written above in detail and with many links (which also link out) about this situation, which is not TSDX-specific and isn't a TS bug either, but a feature many in the TS community would like which TS itself has rejected.

I've started work on a separate Babel plugin to implement this behavior (which was my recommendation beyond the various workarounds), but I haven't had time to finish it up and test it.

agilgur5 avatar Aug 25 '20 03:08 agilgur5