Skip to content

unable to import from installed library in node_modules without relative path #1954

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

Closed
trusktr opened this issue Jul 6, 2021 · 13 comments · Fixed by #2157
Closed

unable to import from installed library in node_modules without relative path #1954

trusktr opened this issue Jul 6, 2021 · 13 comments · Fixed by #2157

Comments

@trusktr
Copy link
Member

trusktr commented Jul 6, 2021

#1679 got closed by the super ugly stupid stale bot (:robot::boom:• • • • :boom::gun:), so continuing here:

I just tried this on 0.19.5 and I still get an error:

import {...} from 'asdom/assembly/index'
> asc assembly/index.ts --target debug --exportRuntime --exportTable

ERROR TS6054: File '~lib/asdom/assembly/index.ts' not found.

 } from 'asdom/assembly/index'
        ~~~~~~~~~~~~~~~~~~~~~~
 in assembly/index.ts(9,8)

FAILURE 1 parse error(s)

This works though:

import {...} from '../node_modules/asdom/assembly/index'

Reproduction:

git clone https://github.com/lume/asdom.git
cd asdom
git checkout unable-to-import
npm install
cd example
npm install
npm run dev // ERROR

The following works, but TypeScript shows red squigglies and intellisense breaks because AssemblyScript projects are not set up by default to satisfy TypeScript node resolution:

import {...} from 'asdom' // red squigglies all over!

The following doesn't work in AS, but it is proper and TypeScript will see the correct imports and we will get working intellisense:

import {...} from 'asdom/assembly/index' // Intellisense works, but not AS
@trusktr
Copy link
Member Author

trusktr commented Jul 6, 2021

Updated the reproduction to checkout a particular branch.

@dcodeIO
Copy link
Member

dcodeIO commented Jul 6, 2021

If assembly is the base directory there, I think a working (for AS) import would be

import {...} from 'asdom/index'

while asdom/assembly/index would attempt to find asdom/assembly/assembly/index.ts. Perhaps that's the culprit we have been looking at here for a while, in that leaving out the assembly would allow it to compile, but is not TS compatible?

@yjhmelody
Copy link
Contributor

@trusktr May be similar to this question MaxGraey/as-string-sink#1

@willemneal
Copy link
Contributor

Why not just?

import {} from "asdom"

@trusktr
Copy link
Member Author

trusktr commented Jul 8, 2021

but is not TS compatible?

Yep, that. Types not found in VS Code. Requires an exports field in package.json to also be compatible with Node-style imports, which TypeScript doesn't support yet.

Why not just?

import {} from "asdom"

A few reasons:

  1. Some things are not exported from the index for various reasons.
  2. That is still not compatible with TypeScript by default, breaks VS Code intellisense out of the box.
  3. For TS/JS-portable code, importing from a package index/main file is bad practice: it imports everything by default and does not tree-shake like the AS compiler does out of the box. This means that by default (for example with native ES Modules) an end user will import everything from a library even if they use only 1% of the library. For example, the statement import {oneLittleThing} from 'some-huge-library' is not so good, especially with big libraries where you don't need everything, because the entire module graph will be imported unnecessarily (and with native ES modules this means more network requests).

@willemneal
Copy link
Contributor

Does types: assembly/index.ts work for 2?

Some things are not exported from the index for various reasons.

So really this issue here is that we can't do asdom/submodule?

Also it sounds like tree shaking is very bad for TS/JS.

@JairusSW
Copy link
Contributor

JairusSW commented Jul 8, 2021

I have the same thing.

import { console } from '../node_modules/as-console/assembly/wasi'

Should be:

import { console } from 'as-console/wasi'

The ../node_module won't work well for people using different package managers (pnpm for example). :'(

@dcodeIO
Copy link
Member

dcodeIO commented Jul 9, 2021

Seems to go into the direction of finding a better convention, and deprecating ascMain - no? :)

@willemneal
Copy link
Contributor

You could keep ascMain for the cases where people don't want assembly/index.ts but regardless of having it as-package/subpackage should resolve to dirname(entry of as-package)/subpackage.

@github-actions
Copy link

github-actions bot commented Aug 8, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

@github-actions github-actions bot added the stale label Aug 8, 2021
@trusktr
Copy link
Member Author

trusktr commented Aug 11, 2021

Not stale. 🤖💥• • • • 💥🔫

@trusktr trusktr added bug and removed stale labels Aug 11, 2021
@dcodeIO
Copy link
Member

dcodeIO commented Aug 11, 2021

It's not purely a bug, as it is kinda by design, but the design itself is kinda a bug, so I guess it is one. We'd need to revert the baseDir computation based on ascMain, which would be a breaking change.

@dcodeIO
Copy link
Member

dcodeIO commented Dec 9, 2021

I've spent some time looking at this problem now, and came to the conclusion that removing ascMain is most likely the most sensible approach. Regardless of whether some of its current problems can be fixed, it will very likely never integrate with existing TS tooling. Without such integration, there'll always be red squigglies all over, either because types cannot be obtained, or wrong types are obtained where TS would do something different than AS.

Hence going to remove the feature, with documentation soon recommending to simply do import from "myPackage/assembly" or any other inner path, as it is an approach that always works.

As a possible path forwards in the future, Node.js nowadays recognizes the exports property in package.json (main is considered legacy iiuc), looking like

  "exports": {
    ".": {
      "import": "./path/to/index.js",
      "require": "./path/to/index.cjs",
      "types": "./path/to/index.d.ts"
    },

where, perhaps, if typical tooling would support it, there could be an "assembly" entry. Likely not going to happen anytime soon, though :)

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

Successfully merging a pull request may close this issue.

5 participants