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

Integer.d.ts #270

Closed
janwo opened this issue Jul 14, 2017 · 21 comments
Closed

Integer.d.ts #270

janwo opened this issue Jul 14, 2017 · 21 comments

Comments

@janwo
Copy link

janwo commented Jul 14, 2017

I just upgraded to the latest npm package and changed the importings accordingly. However...

import Integer from "neo4j-driver/types/v1/integer";
...
if (obj instanceof Integer) return Integer.toNumber(obj);

...results in a error...

Cannot find module 'neo4j-driver/types/v1/integer

Why?

@lutovich
Copy link
Contributor

Hi @janwo,

I do not think you need to change imports. Those TypeScript declaration files are just type declarations, right? They do not contain any actual executable JavaScript code and modules. TypeScript knows how to extract types from NPM package and should use those defined in types folder. Your IDE should also just pick existing TypeScript declarations and make better code completion suggestions.

Hope this helps.

@janwo
Copy link
Author

janwo commented Jul 14, 2017

@lutovich When I make no changes to my imports, I get this error message:

Could not find a declaration file for module 'neo4j-driver/lib/v1/record'. 
'/root/app/node_modules/neo4j-driver/lib/v1/record.js' implicitly has an 'any' type.
`npm install @types/neo4j-driver/lib/v1/record` if it exists or add a new declaration (.d.ts) file containing `declare module 'neo4j-driver/lib/v1/record'`

Typescript 2.4

@lutovich
Copy link
Contributor

@janwo this definitely does not sound right. Could you please share an example project to reproduce this problem?

@lutovich lutovich reopened this Jul 14, 2017
@janwo
Copy link
Author

janwo commented Jul 14, 2017

@lutovich I´ve extracted an example: https://github.com/janwo/neo4j-test

npm install && npm start

@lutovich
Copy link
Contributor

@janwo I'm able to make your code compile by changing imports to:

import neo4j from "neo4j-driver";
import {Driver} from "neo4j-driver/types/v1/driver";
import Integer from "neo4j-driver/types/v1/integer";
import Record from "neo4j-driver/types/v1/record";
import Transaction from "neo4j-driver/types/v1/transaction";
import Session from "neo4j-driver/types/v1/session";
import {AuthToken} from "neo4j-driver/types/v1";

and DatabaseManager#get() function to:

public static get(): Driver {
    const authToken: AuthToken = neo4j.auth.basic("user", "pwd");
    const driver: Driver = neo4j.driver(`bolt://`, authToken);
    if(!DatabaseManager.neo4jClient) DatabaseManager.neo4jClient = driver;
    return this.neo4jClient;
}

I think import {auth, driver} from "neo4j-driver/types/v1"; does not work because this file only contains TS declarations for auth and driver functions. Rest of imports import only type declarations. So actual executable JS code still needs to be imported via import neo4j from "neo4j-driver";.

What do you think?

@janwo
Copy link
Author

janwo commented Jul 14, 2017

I tried this as well. I used that approach when I created this issue. Nevertheless I now pushed the Integer check. And then it does not compile anymore. Actually you are right. There should be no need to change from **/lib/** to **/types/**.

@lutovich
Copy link
Contributor

I'm not entirely sure why this does not work. It has probably something to do with instanceof operator which requires right operand to be a function.

However driver's JS code does not export Integer as a type constructor. It only exports functions to work with integers like int and isInt. Could you please use the following code without instanceof:

if(neo4j.isInt(obj)) {
    const i = obj as Integer;
    console.log('hooray');
}

@janwo
Copy link
Author

janwo commented Jul 14, 2017

But with Record we have the Scenario and it did work. Also the types of @SnappyCroissant made this work. Unfortunately the new types do not allow this. However I am going to use your suggestion now. Thank you for your time :)

@lutovich
Copy link
Contributor

Oh, so it used to work with TypeScript declarations from this PR #183? I'd expect it it work with updated types than, otherwise it's a problem. Do you have an example where Record does not work?

@janwo
Copy link
Author

janwo commented Jul 14, 2017

Yes, the PR is correct. No, sorry. What I meant was the structure of the declaration file of Integer and Record. In both files the class is the default export, but whyever it does only fail in the Integer class.

@pocesar
Copy link
Contributor

pocesar commented Jul 14, 2017

have you tried enabling allowSyntheticDefaultImports in tsconfig.json? this setting is to make interoperable code with non-es modules (lilke this module that is commonjs)

@janwo
Copy link
Author

janwo commented Jul 15, 2017

It says: "Allow default imports from modules with no default export. This does not affect code emit, just typechecking." But all declaration files do have a default export. Also interesting in this case: MikeMcl/bignumber.js#97.

Anyway, I got it working with a minor change in integer.d.ts, see my PR.

@pocesar
Copy link
Contributor

pocesar commented Jul 15, 2017

having a default export doesn't mean it's an ES module. it's just a commonjs with a default member on exports. it exists for interoperability with browser bundlers, like systemjs, webpack, browserify, etc

if you really don't want to use the allowSyntheticDefaultImports setting, you should be using import { default as Integer } from 'neo4j-driver/types/v1/integer'

@janwo
Copy link
Author

janwo commented Jul 15, 2017

@pocesar I tried both variants, both did not work. With allowSyntheticDefaultImports it still returns: Cannot find module 'neo4j-driver/types/v1/integer' for

import Integer from 'neo4j-driver/types/v1/integer';

and

import {default as Integer} from 'neo4j-driver/types/v1/integer';

@janwo
Copy link
Author

janwo commented Jul 15, 2017

I am using @lutovich approach now.

if (Neo4j.isInt(obj)) return obj.toNumber();

obj as Integerdoes not work, as I said, but that's not a big issue. I declared obj as any. This does not feel clean, but for now its a single line in my project, so I am ok with it. Thank you guys anyway. Nevertheless maybe we can make it more compatible some day.

@pocesar
Copy link
Contributor

pocesar commented Jul 15, 2017

oh my bad, I just noticed you're trying to import the types, and that's not meant to happen. Typescript typings are meant to be side-by-side to their .js counterparts, but neo4j team decided to leave them apart, so yes, your only bet was to copy the types to the lib folder (and it will automatically work). the only way to acquire those internal types would be if index.d.ts in v1 folder exported them (it already does export the integer const, but not the types)

@lutovich maybe would be wise to export the interfaces, since they don't mean nothing to the compiled sources, but are useful for people that are consuming and writing libraries that rely on the internals

the lines here in https://github.com/neo4j/neo4j-javascript-driver/blob/1.5/types/v1/index.d.ts#L94-L107 would become:

export {
  driver,
  int,
  isInt,
  integer,
  Neo4jError,
  auth,
  types,
  session,
  error,
  AuthToken,
  Session,
  Integer, // import Integer, { ... } from './integer' at the top
  Config,
  Driver // so it would match the forExport const
}

@lutovich
Copy link
Contributor

lutovich commented Jul 17, 2017

I can confirm that Integer import works when type declarations are in the lib directory together with their javascript counterparts. They were moved to a separate folder just for clarity and to not mix transpiled js and ts. Honestly, I expected presence of types section in package.json to fix all location problems like this. Do you guys know a way to keep type declarations in a different folder and fix this problem with project configuration? For example, Vue has type declarations separately: https://github.com/vuejs/vue/tree/dev/types.

@pocesar I've tried your last suggestion and it does not seem to help.

It is bad to directly import TypeScript declarations, right? Maybe that's another indication they should be moved to lib.

Update: TS manual here seem to say it's fine and ok to place declarations in a separate folder.

@pocesar
Copy link
Contributor

pocesar commented Jul 17, 2017

the issue is to try and import the declarations without a side-by-side js file to back it up. if you are importing typings just for the sake of accessing interfaces, you need to include the .d in the import, like import Integer from 'neo4j-driver/types/v1/integer.d' but that is weird and will be stripped after transpilation, hence why it usually keeps the typings along with the .js files.

when exporting everything, including interfaces, on the main index.d.ts inside types folder, you don't need to rely on importing types paths manually, just doing import {v1} from 'neo4j-driver' then using v1.Integer will work. that's also the reason why TS team decided to not include a typings in package.json that could be an array (see microsoft/TypeScript/issues/6585), to keep everything you need on the file declared in the typings inside package.json (usually the index.d.ts)

so the best approach would be, from the v1/index.d.ts to export everything internal, including the interfaces, so no changes to the old module format would need to take place (as proposed by @janwo in the PR). the old format is declare module 'some/path', and the new 2.0+ is the export / default format. I could make a PR with some improvements, along side taking in consideration that IsInt making use of value is Integer, so inside the scope, TS will recognize the type without extra typecasting (in https://github.com/neo4j/neo4j-javascript-driver/blob/1.5/types/v1/integer.d.ts#L119 declare function isInt(obj: object): boolean; becomes declare function isInt(obj: object): obj is Integer;)

@dmikov
Copy link

dmikov commented Sep 20, 2017

Do you know when it will make to npm? Because right now it takes trial and error to figure out imports in typescript, this is how ridiculous my imports for neo4j look

import {StatementResult} from 'neo4j-driver/lib/v1/result';
import * as neo4j from 'neo4j-driver';
import Record from 'neo4j-driver/types/v1/record';

the usage is not much better:

    const values: any[] = [];
    record.forEach((value) => { values.push(!neo4j.v1.isInt(value) ? value
      : (neo4j.v1.integer.inSafeRange(value) ? value.toNumber() : value.toString()));
    });

@lutovich
Copy link
Contributor

@dmikov we plan a release soon to get this fix and #286 out. Later waits for feedback from internal testing.

@lutovich
Copy link
Contributor

lutovich commented Oct 1, 2017

Fix has been released with 1.4.1 driver which is available on npm.

@lutovich lutovich closed this as completed Oct 1, 2017
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

4 participants