8000 fix(compiler-cli): allow import of modules ending with .js, .ts, etc. by mgechev · Pull Request #14664 · angular/angular · GitHub
[go: up one dir, main page]

Skip to content

fix(compiler-cli): allow import of modules ending with .js, .ts, etc. #14664

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
wants to merge 1 commit into from
Closed

fix(compiler-cli): allow import of modules ending with .js, .ts, etc. #14664

wants to merge 1 commit into from

Conversation

mgechev
Copy link
Member
@mgechev mgechev commented Feb 23, 2017

Fix #14663

Allow importing node modules which name ends with the strings: .ts, .d, .ts, .js, .jsx, .tsx.

According to microsoft/TypeScript#4595
TypeScript knows how to handle imports with an extension so relative
imports should not be a problem (as shown in the tests).

If you prefer to keep the replace, another, alternative solution (less clean I suppose) is:

  moduleNameToFileName(m: string, containingFile: string): string|null {
    // ...
    const original = m;
    m = m.replace(EXT, '');
    const resolve = (m: string) => ts.resolveModuleName(
          m, containingFile.replace(/\\/g, '/'), this.options, this.resolveModuleNameHost)
        .resolvedModule;
    let resolved = resolve(m);
    if (!resolved) {
      resolved = resolve(original);
    }
    return resolved ? this.getCanonicalFileName(resolved.resolvedFileName) : null;
  };

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

AoT doesn't allows import of package names ending with .js, .ts, etc.

What is the new behavior?

AoT allows import of package names ending with .js, .ts, etc.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

@chuckjaz
Copy link
Contributor

What scenario does this enable?

@mgechev
Copy link
Member Author
mgechev commented Feb 23, 2017

When we're importing a symbol from a package which name ends with a string which matches the EXT regular expression.

Here's an example which uses the package called aspect.js:

import { Http } from '@angular/http';
import { Observable } from 'rxjs/Observable';

// Doesn't work because AoT host removes the
// ".js$" from the imported library, which
// causes TypeScript to try to look for the
// package "aspect" instead of "aspect.js".
import { Wove } from 'aspect.js';

// Works with this line because once the AoT host
// drops ".js$" from "aspect.js/aspect.js", the
// TypeScript compiler is able to resolve the file
// "node_modules/aspect.js/aspect.js"
// import { Wove } from 'aspect.js/aspect.js';

@Wove()
export class NameListService {

  constructor(private http: Http) {}

  get(): Observable<string[]> {
    return this.http.get('assets/data.json')
                    .map((res: Response) => res.json());
  }

}

Without the change the @Wove() expression cannot be simplified because the symbol Wove cannot be resolved, since the AoT host removes the /\.js$/ from the package name (aspect.js becomes aspect) and TypeScript cannot resolve the import (since there's no package called aspect in node_modules).

As an attachment I'm providing a demo which can reproduce the problem following the steps:

$ unzip angular-seed.zip
$ cd angular-seed && npm i
$ npm run build.prod.aot

In contrast:

$ npm start

works because there's not AoT compilation involved.

angular-seed.zip

@chuckjaz
Copy link
Contributor

I see. This needs to be limited to v4.0 as we still need to support 1.8 for v2.0 which does not support this. Also, please investigate the e2e failure.

@mgechev
Copy link
Member Author
mgechev commented Feb 23, 2017

I will take a look at the e2e failure during the weekend.

This needs to be limited to v4.0 as we still need to support 1.8 for v2.0...

Does this mean I need to open the PR against different branch?

@chuckjaz
Copy link
Contributor

No I just need to make a note to the caretaker not to cherry-pick this to v2.0. When this is ready I will take care of it.

@cyrilletuzi
Copy link
Contributor

Do we agree that in this case, the compiler will first search for aspect.ts and then, if it does not exist, aspect.js, as TypeScript does since 2.0 ? It's important for compatibility with upcoming <script type="module"></script>, which requires the .js extension.

@mgechev
Copy link
Member Author
mgechev commented Feb 24, 2017

.js is not an extension in this case but part of the package name. If the package name is aspect.ts and TypeScript doesn't find it, it shouldn't look for aspect.js.

@mgechev
Copy link
Member Author
mgechev commented Feb 25, 2017

After looking into the e2e failure, I found that the TypeScript module resolution algorithms treats files with .js extension differently from files with other extensions (PR microsoft/TypeScript#8895 and here's the implementation of this behavior), so the e2e test was trying to resolve app.module.ts.ts.

According to the current behavior, CompilerHost's moduleNameToFileName will remove the extension of it's first argument only when the module is not relative/absolute and the module resolution is not ModuleResolutionKind.Classic.

The current behavior seems compatible with TypeScript 1.8 as well.

Fix #14663

Allow importing node modules which name ends with the strings:

".ts", ".d", ".ts", ".js", ".jsx", ".tsx".

According to microsoft/TypeScript#4595
TypeScript knows how to handle imports with an extension so relative
imports should not be a problem (as shown in the tests).

If you prefer to keep the replace, another, alternative solution (less clean I suppose) is:

```ts
  moduleNameToFileName(m: string, containingFile: string): string|null {
    // ...
    const original = m;
    m = m.replace(EXT, '');
    const resolve = (m: string) => ts.resolveModuleName(
          m, containingFile.replace(/\\/g, '/'), this.options, this.resolveModuleNameHost)
        .resolvedModule;
    let resolved = resolve(m);
    if (!resolved) {
      resolved = resolve(original);
    }
    return resolved ? this.getCanonicalFileName(resolved.resolvedFileName) : null;
  };

```
@chuckjaz chuckjaz added area: core Issues related to the framework runtime type: bug/fix labels May 23, 2017
@vicb
Copy link
Contributor
vicb commented Feb 15, 2018

What is the status of this PR ?

@mgechev
Copy link
Member Author
mgechev commented Feb 15, 2018

Last time I tried aspect.js I didn't face any issues. I think we can close it.

@mgechev mgechev closed this Feb 15, 2018
@mgechev mgechev deleted the fix-import-ext-cli branch February 15, 2018 23:34
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime cla: yes type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could not resolve [moduleName] relative to [path] for modules ending with "*.js"
5 participants
0