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

Missing subfolder & files path completions with wildcard exports #48652

Closed
TheMrZZ opened this issue Apr 12, 2022 · 12 comments · Fixed by #49644
Closed

Missing subfolder & files path completions with wildcard exports #48652

TheMrZZ opened this issue Apr 12, 2022 · 12 comments · Fixed by #49644
Assignees
Labels
Domain: Completion Lists The issue relates to showing completion lists in an editor Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript

Comments

@TheMrZZ
Copy link

TheMrZZ commented Apr 12, 2022

Bug Report

Sub folders and files of a folder listed in exports do not resolve in VS Code's autocomplete.
Also, the modules aren't displayed with a subfolder icon. The files should be displayed with a file icon, but they're not displayed at all.

For this example, I'm using a library named sandstone and a tests folder. I'm trying to import sandstone submodules from tests.

Expected paths & icons:
image

Actual paths & icons:
image

When reaching a submodule (submodules have several .ts files), it gets even weirder. Expected paths:
image

Actual paths:
image
You can see the submodule appears as a submodule of itself.

Despite autocompletion not working, the code runs fine. However, auto imports don't work either. Live demo (ignore the Copilot suggestions):

imports_work.mp4

🔎 Search Terms

typescript, exports, field, package.json, exports map, autocompletion, paths, import, auto import, icons, 4.7 beta

🕗 Version & Regression Information

  • I was unable to test this on prior versions because it's a new 4.7 feature.

💻 Code

Sandstone's package.json:

{
  "name": "sandstone",
  // ...
  "main": "dist/index.js",
  "module": "dist/index.mjs",
  "types": "dist/index.d.ts",
  "exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "import": "./dist/index.mjs",
      "default": "./dist/index.js"
    },
    "./*": {
      "types": "./dist/*.d.ts",
      "import": "./dist/*.mjs",
      "default": "./dist/*.js"
    },
    "./arguments": {
      "types": "./dist/arguments/index.d.ts",
      "import": "./dist/arguments/index.mjs",
      "default": "./dist/arguments/index.js"
    },
    "./commands": {
      "types": "./dist/commands/index.d.ts",
      "import": "./dist/commands/index.mjs",
      "default": "./dist/commands/index.js"
    },
    "./core": {
      "types": "./dist/core/index.d.ts",
      "import": "./dist/core/index.mjs",
      "default": "./dist/core/index.js"
    },
    "./flow": {
      "types": "./dist/flow/index.d.ts",
      "import": "./dist/flow/index.mjs",
      "default": "./dist/flow/index.js"
    },
    "./pack": {
      "types": "./dist/pack/index.d.ts",
      "import": "./dist/pack/index.mjs",
      "default": "./dist/pack/index.js"
    },
    "./variables": {
      "types": "./dist/variables/index.d.ts",
      "import": "./dist/variables/index.mjs",
      "default": "./dist/variables/index.js"
    }
  }
}

Tests' tsconfig.json:

{
  "compilerOptions": {
    "declaration": true,
    "module": "commonjs",
    "moduleResolution": "nodenext",
    "target": "es2020",
    "strict": false,
    "esModuleInterop": true,
    "resolveJsonModule": true,
    "allowSyntheticDefaultImports": true,
    "noEmit": true,
    "skipLibCheck": true,
    "outDir": "build"
  },
  "include": [
    "src/**/*"
  ]
}

🙁 Actual behavior

Only the hardcoded submodules appear in the autocompletion. The submodule appears as a submodule of itself.

🙂 Expected behavior

Since there is a wildcard export, all submodules of submodules & all files should appear in the autocompletion. The submodule shouldn't appear as a submodule of itself.

@andrewbranch andrewbranch self-assigned this Apr 21, 2022
@andrewbranch andrewbranch added the Needs Investigation This issue needs a team member to investigate its status. label Apr 21, 2022
@andrewbranch
Copy link
Member

Related: #47952

@olivier-martin-sf
Copy link

Hi @andrewbranch just saw that #47952 has been merged into main, I gave a try to typescript@next but I am still hitting an issue that is similar to this one. Is #47952 supposed to fix this as well? Just wondering...

On the same topic, is it necessary to change the project config (jsconfig.json) to opt-in for using nodenext for the module compilerOptions property? Or is it something that has been enabled by default? (in the scope of tsserver providing language features).

If that's helpful I have a public repo where a dependency maintained by my team is using exports and where auto-completion for the module paths doesn't work.

@patroza
Copy link

patroza commented Jun 8, 2022

At first I thought maybe this is intentional, sure it is not conforming the behaviour of Node being able to import from the file,
but at least I imagined it could be a performance optimisation so that Typescript can solely rely on the exports without having to scan the file system etc.

But following the Twitter thread, ending up here, I'm also kind of relieved it's considered a bug, as it makes developing in a mono repo rather tedious (can't just add a file and be done with it, must add it to exports too).

Pre Node16 moduleResolution, we would use tsconfig.json paths, like:

        "paths": {
            "@abc/client/*": [
                "./_src/*"
            ],
            "@abc/types/*": [
                "../types/_src/*"
            ],
            "@abc/prelude/*": [
                "../prelude/_src/*"
            ],
            "@abc/prelude": [
                "../prelude/_src"
            ]
        }

but with Node16 resolution this will cause auto imports to add .js.
import { PurchaseOrder } from "@abc/types/PurchaseOrder.js"
while it should be
import { PurchaseOrder } from "@abc/types/PurchaseOrder"

Until you replace package.json

"exports": {
    "./*": {
      "import": {
        "types": "./dist/*.d.ts",
        "default": "./dist/*.js"
      },
      "require": {
        "types": "./dist/*.d.ts",
        "default": "./_cjs/*.cjs"
      }
    }
  }

again with explicit exports.

Also, while using wildcard exports and those tsconfig.json paths entries with Node16 resolution, exposes another problem I run into which is:

../../packages/client/_src/shared/FileView.ts:90:14 - error TS2742: The inferred type of 'FileContainers' cannot be named without a reference to '../../../../node_modules/@abc/types/dist/internal/ids.js'. This is likely not portable. A type annotation is necessary.

../api/_src/Endpoints/Projects/index.ts:37:14 - error TS2742: The inferred type of 'projectRoutes' cannot be named without a reference to '../../node_modules/@abc/client/dist/shared/UserInput.js'. This is likely not portable. A type annotation is necessary.

Which again seems to get fixed once you drop the wildcard exports and use explicit ones instead.

@olivier-martin-sf
Copy link

@andrewbranch is fixing this similar to the work done in #47952 (for the wildcard part)? I am aware that the code paths are different but just wondering how hard this would be to fix and if you have some pointers where the code handling the autocompletion for the language server lives in.

My team at Salesforce is maintaining a package with ~400 entries without main entry points and this issue gets in the way of discoverability for our customers that consume the package. No pressure intended just wondering how hard it would be to fix this and if I could be of any help

@andrewbranch
Copy link
Member

This is on my list to investigate for 4.8. I believe the fix should be pretty similar to #47952, but I suspect that subtle differences in the resolution algorithm could prevent much of the code from being shared. Also be aware that even after this ships, nobody will benefit from it who isn’t using node16/nodenext for their moduleResolution because that’s the only mode that recognizes exports.

@olivier-martin-sf
Copy link

Thx a lot @andrewbranch!

I believe that there's no plan of switching the moduleResolution option to node16 as the default value for the language server before a while is that right? Would make sense considering the current state of the ecosystem

@andrewbranch
Copy link
Member

Correct.

@olivier-martin-sf
Copy link

olivier-martin-sf commented Jun 14, 2022

edit: see @andrewbranch comments below

For anyone stumbling into this (correct me if I am wrong), a temporary workaround is to let your consumers know they should explicitly tell the language server how to resolve those imports by configuring baseUrl and paths.

There's an example for a JS repository there: https://github.com/salesforce/utam-js-recipes/blob/main/jsconfig.json#L5-L10.

This boils down to:

 "compilerOptions": {
        "baseUrl": ".",
        "paths": {
            "salesforce-pageobjects/*": ["node_modules/salesforce-pageobjects/dist/*"]
        }
    }

Note: that paths is relative to the baseUrl options so consumers might have to tweak the values to fit their project structure but that's how we got it working meanwhile.

@andrewbranch
Copy link
Member

What runtime and/or bundler are your users expected to be running?

I don’t think using paths here is great advice, but the easiest way to make it slightly better is to get rid of baseUrl and write your path values as relative to the tsconfig.json file.

@olivier-martin-sf
Copy link

Runtime is node.js (v14 & v16), this repo just demonstrates how to consume and use the package we published (salesforce-pageobjects) to our customers in a sample project. This package holds a set of objects that exposes APIs that describes how to interact with the DOM (context is UI automation testing).

Without configuring paths there's no auto-completion whatsoever when trying to import any file from the salesforce-pageobjects, that's why we came up with this workaround. This might not be the best way to handle it and I am really open to any suggestions.

I am following this as some customers installed our package as a devDependency, tried to use it, and reported it as broken as the auto-completion wasn't working in their IDE.

If that's easier to explain I am also in SF and my DM is open on Twitter (@anchnk)

@andrewbranch
Copy link
Member

If your users are on Node 14/16, they should try to use --moduleResolution node16 if they possibly can, and adding paths is fine as a temporary workaround; I just wouldn’t suggest it as a permanent workaround.

I could also maybe recommend that you add typesVersions mirroring the structure of your exports so that the mapping also applies for your users who are going to be stuck on --moduleResolution node. I can only recommend this because the package crashes on Node versions less than 12 anyway—if it was usable on those older versions of Node, the resolution would be different and users should be required to spell out dist in their paths.

@olivier-martin-sf
Copy link

olivier-martin-sf commented Jun 14, 2022

Thanks a lot, @andrewbranch, and you are right, sorry my comment was confusing: it's indeed meant as a temporary workaround until this issue is closed.

We advised our customers to switch to node16 as soon as TS 4.7 has been released but then they hit the issue mentioned in this thread, hence why we switched back to relying on setting paths. Hope that clears things up.

@andrewbranch andrewbranch added Suggestion An idea for TypeScript Domain: Completion Lists The issue relates to showing completion lists in an editor Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue and removed Needs Investigation This issue needs a team member to investigate its status. labels Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Completion Lists The issue relates to showing completion lists in an editor Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants