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

Issue importing external library #1829

Closed
davedbase opened this issue Feb 22, 2019 · 11 comments
Closed

Issue importing external library #1829

davedbase opened this issue Feb 22, 2019 · 11 comments

Comments

@davedbase
Copy link

davedbase commented Feb 22, 2019

Not sure if this should be possible yet in Deno, but for s* and giggles I tried importing a TypeScript library that does SSR and the import didn't go well. I attempted:

import { Inferno } from 'https://raw.githubusercontent.com/infernojs/inferno/master/packages/inferno/src/index.ts'

It retrieved the index but seems to have failed with the first dependency:

Downloading https://raw.githubusercontent.com/infernojs/inferno/master/packages/inferno/src/index.ts...
Downloading https://raw.githubusercontent.com/infernojs/inferno/master/packages/inferno/src/core/types... NOT FOUND
Uncaught NotFound: Cannot resolve module "./core/types" from "/Users/ddibiase-macbook/Library/Caches/deno/deps/https/raw.githubusercontent.com/infernojs/inferno/master/packages/inferno/src/index.ts"

The dependency is definitely available: https://raw.githubusercontent.com/infernojs/inferno/master/packages/inferno/src/core/types.ts. It also looks like it resolved the path properly but then just failed to retrieve it.

Not sure if this is a bug or if I'm not supposed/can't use the import this way.

@bartlomieju
Copy link
Member

Actually it's expected. Deno requires explicit extensions in imports (Deno does not try to guess if it is a JS or TS file) and inferno/index.ts references export * from './core/types';
So Deno looks for inferno/src/core/types file and it does not exist but it's actually inferno/src/core/types.ts.

@davedbase
Copy link
Author

Ok, that's what I was expecting. I want to bring up the point that there are a number of libraries that already exist out there within Typescript (such as Inferno) that could relatively easily be imported if this weren't an issue.

As an Inferno team developer for us to support Deno we would have to adjust quite a bit to make it available. Matter of fact I will be taking the changes back to the Inferno core team to see if they are feasible.

Would it make sense to create a hook within Deno to tap into the import process? Would be helpful to control the import paths and make import changes dynamically/on the fly. That would make solving this issue easy. ie. for every new import the hook could be taught to append .ts to the end. This would allow for more flexibility in the import procedure and make supporting existing libraries easier.

Side note: it would be interesting if the Deno JS had an API for tracking imports. I'm imaging a JS-based callback system that occurs as a prelude to the actual app booting up. Events/callsback are triggered when: a) an import begins, b) an import completes, c) the progress of a library import and all its dependencies, d) when a cached version of the library is used, e) methods to dynamically purge a cache and re-import a new dependency.

In the Node.js world an eye-watering ecosystem of cache management plugins popped up over time.

@bartlomieju
Copy link
Member

CC @ry @kitsonk

@kevinkassimo
Copy link
Contributor

This has been a recurring topic. At one point Deno used to have an auto extension-appending guessing mechanism, but was dropped in #1396 in favor of import strategies that has no magic.

I think it might make sense to introduce custom import APIs, which might also help with resolutions to new formats (something like Reason?), compatibility to some NPM packages, and custom impl of import-map independent from Deno core.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 22, 2019

Maybe the one thing that can be done is that if a module id is remote and doesn't have an extension that it inherits the extension of the referrer. We would then support this scenario with zero configuration and almost no performance impact.

I think import-map is too early to place our bets on.

@davedbase
Copy link
Author

Further to the above, importing roots of a library is another issue. To get the Inferno example going I've cloned the source and I'm manually replacing the imports cries but realized this will be trickier:

import { warning } from 'inferno-shared';

The import is basically expecting to resolve the root export of the library.

Tangentially, have you guys made a decision how to handle typings? Building on the inferno-shared reference above, a package.json would have a typings property ie. https://raw.githubusercontent.com/infernojs/inferno/master/packages/inferno-shared/package.json. Just wondering what the best approach to work around that is (aside from changing the few hundred import references in the library :p).

Also for clarity, definitely not vying for going back to the old ways of doing things. Just wondering if @ry or the broader Deno community has a plan for tackling these use cases. If note I'm sure they'll come in time, just curious.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 23, 2019

The problem we are running against is that bare specifiers like:

import { warning } from 'inferno-shared';

Is specifically not supposed to work in a browser (see: #1830). import-maps sort of would allow that. It also requires auto-magic module resolution to find some sort of index.ts which is very much a non-goal.

The types are irrelevant when you are loading the TypeScript source. Certainly introspecting a package.json for any information is very anti-pattern.

Then again, we are going to be running into this again and again. While import-maps isn't fully baked yet, it may actually make sense to consider implementing, as I don't see any other reasonable way and it is on a standards track. It would solve all these use cases we are talking about here.

@davedbase
Copy link
Author

davedbase commented Feb 24, 2019

I think import-maps makes the most sense rn.

Just remembered that TS supports the paths property.

A lot of TS libraries seem to use this and it does roughly the same thing as import-maps (I think)? Here's an example from Inferno: https://github.com/infernojs/inferno/blob/master/tsconfig.json. Does it make sense to allow domain-based paths listed in the tsconfig and have Deno resolve them?

If that were possible AND if the extensions of scripts were appended based on the root script being imported, that should solve at least my edge case. I suppose the solution is indeed anti-pattern though.

Aside from that compiler still in flux, support for custom tsconfig files is still on hold so this is a mute point until that gets resolved.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 24, 2019

In Deno, the paths option has no effect as we replace all the module resolution. You can use paths with standard tsc or VScode to partially mimic Deno actually. paths came out of the need to mimic configurable loaders like AMD and SystemJS. import-maps also takes hints from those two loaders. My main concern is that the WHATWG loader spec appears stalled which was also solving this issue.

If import-maps is realistically going forward with other user agents then it feels like the right thing to do.

cc/ @guybedford any thoughts?

@kitsonk
Copy link
Contributor

kitsonk commented Mar 12, 2019

I think we could close this in lieu of #1921

@bartlomieju
Copy link
Member

Import map support is available in Deno starting with v0.9.0 I suppose we can close this issue.

CC @ry

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

No branches or pull requests

5 participants