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

Error importing buildt ts files with allowJs #667

Closed
Pajn opened this issue Oct 24, 2017 · 10 comments · Fixed by #674
Closed

Error importing buildt ts files with allowJs #667

Pajn opened this issue Oct 24, 2017 · 10 comments · Fixed by #674

Comments

@Pajn
Copy link
Contributor

Pajn commented Oct 24, 2017

We have a subproject that is built separately so that it produces .js and .d.ts files. With "allowJs": true I get the following error when importing a module from it:
TS2306: File /path/to/file.js is not a module.

This can be fixed by setting entryFileCannotBeJs: true but as that is going away I think this issue must be fixed first.

I have not looked deeper into it but the feeling I get is that ts-loader prefer the .js file over the .d.ts and thus errors. Plain TypeScript have not errors with this setup.

@johnnyreilly
Copy link
Member

Please try upgrading to ts-loader v3

@Pajn
Copy link
Contributor Author

Pajn commented Oct 24, 2017

Sorry, I forgot to specify that this is with ts-loader v3.
I saw this with 3.0.3 so I upgraded to 3.0.5 but the problem persisted. I have not tried older versions than 3.0.3.

@johnnyreilly
Copy link
Member

Okay - please could you supply a minimal repo that illustrates the issue? We need that so we can work on resolving the issue. Thanks.

@Pajn
Copy link
Contributor Author

Pajn commented Oct 24, 2017

Here is one https://github.com/Pajn/ts-loader-repro
It does not produce exactly the same error, but I'm pretty sure the error is created by the same reason.

Running yarn build in main-module in the repro produces this output:

yarn run v1.2.1
$ webpack
Hash: 7ebbb87e5a6af7011999
Version: webpack 3.8.1
Time: 699ms
 Asset     Size  Chunks             Chunk Names
app.js  2.88 kB       0  [emitted]  main
   [0] multi ./src/index.ts 28 bytes {0} [built]
   [1] ./src/index.ts 139 bytes {0} [built] [1 error]
   [2] ../other-module/build/module.js 69 bytes {0} [built]

ERROR in ./src/index.ts
[tsl] ERROR in /Users/rasmus/Development/ts-loader-repro/main-module/src/index.ts(1,9)
      TS2305: Module '"/Users/rasmus/Development/ts-loader-repro/other-module/build/module"' has no exported member 'Name'.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Running tsc in main-module does not error

@johnnyreilly
Copy link
Member

First of all: thanks for the repro - I really appreciate it.

However since this isn't the same error as the one you initially reported I'm a little hesitant to take a look. I have opened it up to take a look and this looked a little strange to me:

import {Name, name} from '../../other-module/build/module'

I'm not totally sure what behaviour you would expect here as you're moving up past the root of the typescript project. I'm not totally clear what the valid behaviour would be. Do you want to tweak the project so it works by using file references in the package.json? See the example here

I think if you use that approach you might be able to come up with a repo that illustrates the issue. Failing that, maybe yarn link is worth a try?

Thanks again!

@Pajn
Copy link
Contributor Author

Pajn commented Oct 24, 2017

I can probably produce a repro with the same error as well, but it might be more convoluted when taken out of context.

File references does not work for us as we develop both modules simultaneously and can't reinstall the modules for every simple change.
I'm aware of linking, but that does introduce extra complexity as a simple yarn isn't enough to set up the dependencies which is why we have chosen to avoid it in smaller projects. Yarn workspaces will probably be a good solution when that is ready but so far this has worked fine for us.

Plain Typescript deals with it without issues, so I can not see why it should not work with ts-loader. If I ignore the type error, there is no other issues as well meaning that Webpack is fine with it as well.

By the behavior and errors (without diving into the code) it seems to me that the error is that ts-loader does not use the .d.ts file when "allowJs": true and therefore errors for type imports as those exports are not present in the .jsfile, or (which is my first error) errors for the file not being a module at all when all exports are dynamic so it can not infer any exported property from the .js file.

@johnnyreilly
Copy link
Member

johnnyreilly commented Oct 24, 2017

FWIW I think the general issue is probably connected with this part of the ts-loader code: https://github.com/TypeStrong/ts-loader/blob/master/src/servicesHost.ts#L219

Bearing in mind what you said I tried to have a quick play but I'm afraid the repro repo seems to be problematic. I get the following error when I try to yarn build as instructed:

Hash: d72599a9383dff8b8d93
Version: webpack 3.8.1
Time: 56ms
 Asset     Size  Chunks             Chunk Names
app.js  2.83 kB       0  [emitted]  main
   [0] multi ./src/index.ts 28 bytes {0} [built]
   [1] ./src/index.ts 239 bytes {0} [built] [failed] [1 error]

ERROR in ./src/index.ts
Module parse failed: Unexpected token (3:7)
You may need an appropriate loader to handle this file type.
| import {Name, name} from '../../other-module/build/module'
|
| const n: Name = name
| console.log(n)
 @ multi ./src/index.ts
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

You might want to take a look and refine the repo...

@johnnyreilly
Copy link
Member

Thinking some more I wonder if this is actually the issue: https://github.com/TypeStrong/ts-loader/blob/master/src/servicesHost.ts#L187

        if (resolvedFileName.match(scriptRegex)) {
            resolutionResult = { resolvedFileName, originalFileName };
        }

scriptRegex above may either be /\.tsx?$|\.jsx?$/i or /\.tsx?$/i. It will always be /\.tsx?$/i in the case that that entryFileCannotBeJs: true. I'm going to speculate that if we only tested for /\.tsx?$|\.jsx?$/i when the path does not contain node_modules then all would work as you might hope. Could be worth you forking and trying this out locally?

@Pajn
Copy link
Contributor Author

Pajn commented Oct 26, 2017

I dug a bit in the code.

Long story short, by changing the regex in isJsImplementationOfTypings to consider all .d.ts files and not just those in node_modules, it works.

Long story:
When it get to that check resolvedFileName as well as originalFileName is the .js file as resolveSync prefers the .js file over the .d.ts file. If I change my webpack config so that .d.ts is listed before .js there is no longer any error from TypeScript, but then the compiled file is bad as webpack have bundled the .d.ts file instead of the .js file.
If I keep my webpack config as is (no .d.ts in extensions), tsResolution gets resolved correctly to the .d.ts file but resolutionStrategyTS24AndAbove (I have not tested with older Typescript) returns resolutionResult because of the mentioned regex in isJsImplementationOfTypings.

I'll submit a PR to discuss it

@johnnyreilly
Copy link
Member

Awesome - thanks so much! I'll try and take a look tomorrow. 🌻

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.

2 participants