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

Module imports need to recognize/ignore the file extension #4595

Closed
weswigham opened this issue Sep 1, 2015 · 9 comments
Closed

Module imports need to recognize/ignore the file extension #4595

weswigham opened this issue Sep 1, 2015 · 9 comments
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@weswigham
Copy link
Member

Currently, TS can't find modules when you use a file extension in the import name (import {Foo} from './foo.ts';) - SystemJS updated and file extensions are now required (though the spec issue is still under discussion) on imports (without setting some legacy settings in System.config). So when compiling with --module, you can either have a functioning typechecker (no extensions on your imports) or a functioning runtime (.js on your imports, since your result files have .js extensions). This is complicated further by systemjs's bundler, which looks for dependent files before TS transpiles them, so you have to use .ts extensions on your imports so the bundler can resolve the paths to real ones.

This extension madness is all incredibly awkward to work with, and I'm not sure if this needs module resolution logic updating on our part, arguing for old behavior on the loader spec, or talking to systemjs and having them change how they're finding modules ignoring the spec.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 2, 2015

I hate to say this (and having read through the thread you linked to) that is madness. Are TC39 going to seriously consider adopting that? My initial reaction is now is the time to abandon SystemJS before it gets too late. I think @jrburke tried (sadly fruitlessly) to explain why that approach is going to cause all sorts of problems. Transpilation (and ergo TypeScript) are already going to suffer from this now. There is a very good reason why the MID should be decoupled from the actual physical filesystem/URI. As you are pointing out, bundling and build optimisation, transpilation, etc can make a MID with an extension something totally inane.

This also throws up whole issues with those of us using CommonJS/AMD/UMD. TypeScript is going to have to try to start trying to rewrite MIDs based on the packaging target...

My advice would be to wait at least to see what comes out of TC39 in regards to this. If that gets adopted then, it gets adopted and we all have to deal with it. Does anyone know what the plan with TC39 is?

@jrburke
Copy link

jrburke commented Sep 2, 2015

My understanding is that the loader spec will be setting the constraints around module ID resolution. Since that is a whatwg repo, it is not something that is specifically under the TC39 umbrella, but as far as I know the people authoring the spec are TC39 members.

If you wanted to discuss with them, opening tickets in the whatwg loader repo seems to be the main channel, although response times by authors can be long. I have also asked if I could help or offered to talk through issues offline, but those did not receive responses. It seems they prefer to consult among themselves as they feel they have the most global context, and perhaps browser implementers, before engaging more with others outside of that core.

I am unsubscribing from this issue, so may not see further updates.

@mhegazy mhegazy added the Bug A bug in TypeScript label Sep 2, 2015
@IgorMinar
Copy link

Have you considered accepting imports authored without .ts extension and emitting imports with .js extension?

With this setup, the systemjs bundler could then be fed es6 code which already contains imports with the extension and things would "just work".

I don't quite understand the reasoning behind these recent loader spec/systemjs changes despite reading quite a bit about it. For application code it makes sense to make the imports environment specific (except for the extension which changes with transpilation), but for 3rd party code, I'd expect the imports to be portable so that I can easily switch between local development friendly copy of the lib and a production, minified, and/or cdn hosted version of it without changing any imports in my code.

@IgorMinar
Copy link

related SystemJS issue: systemjs/systemjs#756

@frankwallis
Copy link
Contributor

TypeScript 1.6.0-beta now supports overriding the built in module resolver as per this issue. I have implemented this in plugin-typescript so that it uses the systemjs resolution logic which means you can currently import files with or without the .ts extension.

I agree with @IgorMinar that having TypeScript change the file extensions when compiling may be the best way to support this, but for that I think that SystemJS would need to provide a way for loaders to change the extensions of the files as they move through the pipeline. It does seem odd that currently after compilation you have .ts files in the registry which have been converted to JavaScript.

@IgorMinar
Copy link

I’m looking at #8895 that was just merged and I’m concerned about this module ID resolution change.

While it solves the original problem discussed in here it does so in a way that requires changes to all of the user code if they want to take advantage of this change. What’s worse, the node.js community is having heated debates about adopting “.mjs” extension for es modules (ESM) which would then require the typescript to change the module resolution once again and users would have to change their code once again too. Doing so would also lock the code base to be either commonjs or ESM compatible. This is pretty crazy.

If I'm not missing anything, I would recommend reverting that change and either waiting for the module loader spec to stabilize and node.js to decide on how they want to do this, or add an configuration option which will allow the users to control what if any extension should be appended to all imports within a given compilation unit (excluding all external module imports).

@mhegazy
Copy link
Contributor

mhegazy commented Jun 9, 2016

. What’s worse, the node.js community is having heated debates about adopting “.mjs” extension for es modules (ESM) which would then require the typescript to change the module resolution once again

If and when the ".mjs" decision is finalized, we will add .mjs to the list of extensions. so you can write your imports to .mjs files and it should work. I do not think this issue make the change any different we will need to add a new module target, and emit files in .mjs instead of .js etc..

If I'm not missing anything, I would recommend reverting that change and either waiting for the module loader spec to stabilize and node.js to decide on how they want to do this, or add an configuration option which will allow the users to control what if any extension should be appended to all imports within a given compilation unit (excluding all external module imports).

The rational here is your valid JS code should be valid TS code. that is not the case with imports. in JS you can write import "foo.js", import "foo.mjs" or just import "foo", with this change, the compiler will understand where to find the types regardless.

The fundamental issue here, we do not believe the compiler should rewrite your imports. module names are resource identifiers, and the compiler should not mess with them. all what it needs is to know where the definition for this module lives, and we have added a series of features to make this possible, e.g. node module resolution logic, path mapping resolutions, rootDirs support, @types support, and now this change.

Given that the compiler will not rewrite your imports, i do not see how this change makes any thing any worse, if not better.

@billti
Copy link
Member

billti commented Jun 9, 2016

I would note that while browsers seem to be requiring the file extension, the current Node plan is that you still omit the extension and it has the list it iterates through looking for (see https://github.com/nodejs/node-eps/blob/master/002-es6-modules.md#51-determining-if-source-is-an-es-module ). So the authored code wouldn't change, but our resolution algorithm would.

@IgorMinar
Copy link

Interesting way to look at this problem. @mhegazy thanks for the rationale. That point really clarifies how you are thinking about this.

I always thought of the modules ids in ts code as "design-time" identifiers. I will try to readjust my mental model to match yours.

@billti that is correct but this kind of resolution relies on js (cjs) and mjs (esm) interop which is being proposed for node, but won't work for tools that require ESM only (e.g. rollup). Those tools will need to read in the file to determine if the content is acceptable. It's extra work, but I think that's fine.

Thanks for the clarification.

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

7 participants