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

Added speculative test of allowJsImportTypes scenario (should initially fail until fix is applied) #590

Merged
merged 8 commits into from
Jul 26, 2017

Conversation

johnnyreilly
Copy link
Member

No description provided.

@johnnyreilly
Copy link
Member Author

Took a while to get there but we now have a test that is (I think) failing for the correct reason. Now to apply the fix and see where that takes us.

…ated as isExternalLibraryImport

Suggested by @bsouthga - let's see if this fixes our broken test.
@johnnyreilly
Copy link
Member Author

This looks pretty good - seems like the proposed change resolves the issue. The test doesn't pass on TypeScript 1.8 / 2.0 but I think that's probably down to the test itself. When I get a little more time I'll give this a more thorough test and find out why. Since this doesn't seem to break any existing functionality I may look to merge this.

@johnnyreilly
Copy link
Member Author

Ah - total failure on Windows. Think that's down to the regex

@johnnyreilly
Copy link
Member Author

@bsouthga - if you get a moment could you give this fork a test?

@johnnyreilly
Copy link
Member Author

It's possible that with this in place we may no longer need the entryFileIsJs loader option - this behaviour could be driven purely by whether allowJs or checkJs was set. I have a feeling that issue this PR resolves was the reason this was hidden behind a flag. See here:

#388

Will look to experiment with this later.

@bsouthga
Copy link

Hi John, thanks for taking another look at this -- sorry for not following up, our specific issue was fixed by changing the entry to .ts. I'll take a peek at your changes.

@johnnyreilly
Copy link
Member Author

Thanks @bsouthga - that'd be great!

@bsouthga
Copy link

It looks like that fixed the initial issue ("no exported member") in both the test repo and our private codebase -- however, now I get several issues like this:

ERROR in <omitted>/lib/both/batch.ts
(1,29): error TS7016: Could not find a declaration file for module 'dataloader'. '<omitted>/node_modules/dataloader/index.js' implicitly has an 'any' type.
  Try `npm install @types/dataloader` if it exists or add a new declaration (.d.ts) file containing `declare module 'dataloader';`

ERROR in <omitted>/app/both/adapters/telemetry.ts
(1,30): error TS7016: Could not find a declaration file for module 'crosslytics'. '<omitted>/node_modules/crosslytics/lib/index.js' implicitly has an 'any' type.
  Try `npm install @types/crosslytics` if it exists or add a new declaration (.d.ts) file containing `declare module 'crosslytics';`

ERROR in <omitted>/app/client/tm/clPlatform.tm.register.ts
(2,31): error TS7016: Could not find a declaration file for module 'react2angular'. '<omitted>/node_modules/react2angular/index.js' implicitly has an 'any' type.
  Try `npm install @types/react2angular` if it exists or add a new declaration (.d.ts) file containing `declare module 'react2angular';`

ERROR in ./app/client/_shared/services/global/crosslytics.service.ts
(1,29): error TS7016: Could not find a declaration file for module 'crosslytics'. '<omitted>/node_modules/crosslytics/lib/index.js' implicitly has an 'any' type.
  Try `npm install @types/crosslytics` if it exists or add a new declaration (.d.ts) file containing `declare module 'crosslytics';`

The common thing across all those libs is that they have bundled typings listed in <package.json>.typings. Unfortunately I haven't been able to create a small isolated repro though, so its possible this is something specific to us.

@bsouthga
Copy link

bsouthga commented Jul 26, 2017

For posterity, our tsconfig.json is this:

{
  "compilerOptions": {
    "target": "es5",
    "module": "esnext",
    "moduleResolution": "node",
    "strict": true,
    "skipLibCheck": true,
    "sourceMap": true,
    "allowJs": true,
    "outDir": "dist",
    "lib": [
      "es2015",
      "dom"
    ],
    "jsx": "react",
    "types": [
      <some global types>
    ]
  },
  "include": [
    "./**/*.ts",
    "./**/*.tsx",
    "../../dev/typings/declarations.client.d.ts",
    "../../dev/typings/declarations.both.d.ts",
    "../../dev/typings/lodash-3.10.d.ts",
    "../../dev/typings/tyranid-schemas-client.d.ts",
    "../../dev/typings/tyranid-schemas-isomorphic.d.ts"
  ],
  "exclude": [
    "node_modules",
    "./**/*.{spec,test}.ts"
  ],
}

@johnnyreilly
Copy link
Member Author

Thanks for that @bsouthga - I'd appreciate any more information on that you might have. A repro would be awesome. I'm going to merge this as I think it's an improvement on what was there before. Thanks for your help with this - it's really good of you.

In the longer term I think we could probably retire the overt entryFileIsJs option and just infer it based on allowJs being set in the tsconfig.json. However, before I take that step I want to make sure that the scenario you've mentioned here is covered - otherwise we could break lots of people...

@johnnyreilly
Copy link
Member Author

@bsouthga it looks like the issue you're now experiencing may be down to an issue in TypeScript's resolution mechanism; see here: microsoft/TypeScript#9854

There are workarounds here: https://stackoverflow.com/questions/41292559/could-not-find-a-declaration-file-for-module-module-name-path-to-module-nam

I don't know if they help you?

@bsouthga
Copy link

Thanks for the links, that is helpful.

unfortunately it looks like it might be something else for us, as all those libs seem to have the correct main/typings setup (per the SO answer).

It's very possible there is something else going on, as our TS setup is pretty complicated, I'll keep digging around and report back if there is a clear ts-loader related issue.

@johnnyreilly
Copy link
Member Author

Thanks @bsouthga - I appreciate that.

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

Successfully merging this pull request may close these issues.

None yet

2 participants