etesync / etebase-js

Etebase TypeScript API for the web, node and react-native!
https://www.etebase.com
BSD 3-Clause "New" or "Revised" License
159 stars 11 forks source link

Etebase library does not compile with "tsconfig.json:compilerOptions.strict: true" #15

Open hlovdal opened 3 years ago

hlovdal commented 3 years ago

Trying to use the etebase library in an Angular project initialized with strict option fails. Steps to reproduce:

  1. Create minimal project:
$ cd /tmp
$ ng new --defaults --strict test
CREATE test/README.md (1013 bytes)
...
CREATE test/e2e/src/app.po.ts (274 bytes)
✔ Packages installed successfully.
    Successfully initialized git.
$ cd test
$
  1. Install and use etebase.
$ npm install --save etebase
...
+ etebase@0.42.0
added 11 packages from 13 contributors and audited 1626 packages in 14.594s
...
$ $EDITOR src/app/app.component.ts
...
$ git diff src/app/app.component.ts
diff --git a/src/app/app.component.ts b/src/app/app.component.ts
index 29d65ec..bdd1e6a 100644
--- a/src/app/app.component.ts
+++ b/src/app/app.component.ts
@@ -1,4 +1,5 @@
 import { Component } from '@angular/core';
+import { Account } from 'etebase';

 @Component({
   selector: 'app-root',
@@ -7,4 +8,7 @@ import { Component } from '@angular/core';
 })
 export class AppComponent {
   title = 'test';
+  public test(): void {
+    Account.login("username", "password", "url");
+  }
 }
$
  1. Running test now fails:
$ npm run test -- --watch=false

> test@0.0.0 test /tmp/test
> ng test "--watch=false"
...
✔ Browser application bundle generation complete.

Error: node_modules/etebase/dist/lib/Crypto.d.ts:1:21 - error TS7016: Could not find a declaration file for module 'libsodium-wrappers'. '/tmp/test/node_modules/libsodium-wrappers/dist/modules/libsodium-wrappers.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/libsodium-wrappers` if it exists or add a new declaration (.d.ts) file containing `declare module 'libsodium-wrappers';`

1 import _sodium from "libsodium-wrappers";
                      ~~~~~~~~~~~~~~~~~~~~

Error: node_modules/etebase/dist/lib/Helpers.d.ts:1:21 - error TS7016: Could not find a declaration file for module 'libsodium-wrappers'. '/tmp/test/node_modules/libsodium-wrappers/dist/modules/libsodium-wrappers.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/libsodium-wrappers` if it exists or add a new declaration (.d.ts) file containing `declare module 'libsodium-wrappers';`

1 import _sodium from "libsodium-wrappers";
                      ~~~~~~~~~~~~~~~~~~~~

Error: node_modules/etebase/dist/lib/OnlineManagers.d.ts:1:17 - error TS7016: Could not find a declaration file for module 'urijs'. '/tmp/test/node_modules/urijs/src/URI.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/urijs` if it exists or add a new declaration (.d.ts) file containing `declare module 'urijs';`

1 import URI from "urijs";
                  ~~~~~~~

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! test@0.0.0 test: `ng test "--watch=false"`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the test@0.0.0 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     $HOME/.npm/_logs/2021-03-03T20_38_58_942Z-debug.log
$
  1. Installing the missing type packages makes the errors a bit less obscure, but does not fix the underlying problem:
$ npm i --save-dev @types/libsodium-wrappers
...
+ @types/libsodium-wrappers@0.7.9
added 1 package from 1 contributor, removed 1 package and audited 1627 packages in 15.381s
...
$ npm i --save-dev @types/urijs
...
+ @types/urijs@1.19.14
added 1 package from 10 contributors and audited 1628 packages in 14.163s
...
$ npm run test -- --watch=false
...
✔ Browser application bundle generation complete.

Error: node_modules/etebase/dist/lib/Crypto.d.ts:1:8 - error TS1192: Module '"/tmp/test/node_modules/@types/libsodium-wrappers/index"' has no default export.

1 import _sodium from "libsodium-wrappers";
         ~~~~~~~

Error: node_modules/etebase/dist/lib/Helpers.d.ts:1:8 - error TS1192: Module '"/tmp/test/node_modules/@types/libsodium-wrappers/index"' has no default export.

1 import _sodium from "libsodium-wrappers";
         ~~~~~~~

Error: node_modules/etebase/dist/lib/OnlineManagers.d.ts:1:8 - error TS1259: Module '"/tmp/test/node_modules/@types/urijs/index"' can only be default-imported using the 'allowSyntheticDefaultImports' flag

1 import URI from "urijs";
         ~~~

  node_modules/@types/urijs/index.d.ts:26:1
    26 export = URI;
       ~~~~~~~~~~~~~
    This module is declared with using 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag.

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! test@0.0.0 test: `ng test "--watch=false"`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the test@0.0.0 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     $HOME/.npm/_logs/2021-03-03T20_40_06_933Z-debug.log
$

If the project is created with just ng new --defaults test then no errors are generated. The main difference between strict and unstrict is the following difference in tsconfig.json:

--- test/tsconfig.json  2021-03-03 21:57:54.304950915 +0100
+++ test.strict/tsconfig.json   2021-03-03 21:59:09.016755082 +0100
@@ -4,6 +4,10 @@
   "compilerOptions": {
     "baseUrl": "./",
     "outDir": "./dist/out-tsc",
+    "forceConsistentCasingInFileNames": true,
+    "strict": true,
+    "noImplicitReturns": true,
+    "noFallthroughCasesInSwitch": true,
     "sourceMap": true,
     "declaration": false,
     "downlevelIteration": true,
@@ -18,6 +22,9 @@
     ]
   },
   "angularCompilerOptions": {
-    "enableI18nLegacyMessageIdFormat": false
+    "enableI18nLegacyMessageIdFormat": false,
+    "strictInjectionParameters": true,
+    "strictInputAccessModifiers": true,
+    "strictTemplates": true
   }
 }

Testing with a project created with without --strict but just adding "strict": true, to compilerOptions in tsconfig.json also produces the errors, thus compilerOptions.strict: true is the triggering factor.

hlovdal commented 3 years ago

Just setting strict false in my initial project did not help for some reason, but I got things compiling by changing

-import _sodium from "libsodium-wrappers";
+import _sodium = require("libsodium-wrappers");

in node_modules/etebase/dist/lib/Crypto.d.ts and node_modules/etebase/dist/lib/Helpers.d.ts, and

-import URI from "urijs";
+import URI = require("urijs");

in node_modules/etebase/dist/lib/OnlineManagers.d.ts although depending on manually modified files inside node_modules is not great...

tasn commented 3 years ago

I tested it with strict: false and it works, though I wonder if there's maybe something else at play here. Maybe a different typescript version? (though we should be on a fairly recent one).

Changing to require is not a solution (and I'm surprised the import ... require() syntax even works), because it would just remove the type information altogether. These types shouldn't be exposed in the API anyway (they are for internal modules), but it's still interesting/odd.

I think I fixed it, I just pushed a new branch (called typing). Can you please verify?

hlovdal commented 3 years ago

The require part is a red herring because I am unable to re-trigger the issue in my main project even after removing both node_modules and dist directories, and then doing a plain npm install with same versions as earlier but now without errors...

But it is still reproducible with the minimal project, so I tested with that. I'm leaving out most of the command output here, it is similar to previously.

cd /tmp/
ng new --defaults --strict test
cd test
npm install --save etebase
$EDITOR src/app/app.component.ts
npm run test -- --watch=false
# Fails as before
cd ..
git clone https://github.com/etesync/etebase-js
cd etebase-js/
# Not switching branch yet, so latest master commit
yarn install
cd ../test/
mv node_modules/etebase/dist{,.orig}
mv ../etebase-js/dist node_modules/etebase/dist
npm run test -- --watch=false
# Again fails as before.
cd ../etebase-js/
git checkout typing origin/typing 
git checkout -b typing origin/typing 
yarn 
cd ../test/
mv node_modules/etebase/dist{,.master}
mv ../etebase-js/dist node_modules/etebase/dist
npm run test -- --watch=false
...

Error: node_modules/etebase/dist/lib/OnlineManagers.d.ts:1:17 - error TS7016: Could not find a declaration file for module 'urijs'. '/tmp/test/node_modules/urijs/src/URI.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/urijs` if it exists or add a new declaration (.d.ts) file containing `declare module 'urijs';`

1 import URI from "urijs";
                  ~~~~~~~

...

So the two libsodium errors are gone, but there is still one urijs left.

tasn commented 3 years ago

Yeah, those were were definitely fixed. I also changed the package.json to require @types/urijs as a dep. Is it installed for you? (yarn why @types/urijs) Does it get fixed if you manually install it?

hlovdal commented 3 years ago

In the last example @types/urijs was not installed, but installing it only gives a more descriptive error, same as in initial comment:

  node_modules/@types/urijs/index.d.ts:26:1
    26 export = URI;
       ~~~~~~~~~~~~~
    This module is declared with using 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag.
tasn commented 3 years ago

Gotcha, so it's actually a problem with urijs's types. Is this with the latest types? Can you check with them maybe? I'll try to think how I can remove it.

hlovdal commented 3 years ago

Yes, this seems to be a pure urijs problem. I tested compiling with compilerOptions.traceResolution set to true, and it shows that the compiler tries to find a non-existing URI library file as well.

...
======== Resolving module 'urijs' from '/tmp/test/node_modules/etebase/dist/lib/OnlineManagers.d.ts'. ========
Explicitly specified module resolution kind: 'NodeJs'.
'baseUrl' option is set to '/tmp/test', using this value to resolve non-relative module name 'urijs'.
Resolving module name 'urijs' relative to base url '/tmp/test' - '/tmp/test/urijs'.
Loading module as file / folder, candidate module location '/tmp/test/urijs', target file type 'TypeScript'.
File '/tmp/test/urijs.ts' does not exist.
File '/tmp/test/urijs.tsx' does not exist.
...
File '/tmp/test/node_modules/@types/urijs/index.d.ts' exist - use it as a name resolution result.
Resolving real path for '/tmp/test/node_modules/@types/urijs/index.d.ts', result '/tmp/test/node_modules/@types/urijs/index.d.ts'.
======== Module name 'urijs' was successfully resolved to '/tmp/test/node_modules/@types/urijs/index.d.ts' with Package ID '@types/urijs/index.d.ts@1.19.14'. ========
...
======== Resolving module 'URI' from '/tmp/test/node_modules/@types/urijs/index.d.ts'. ========
Explicitly specified module resolution kind: 'NodeJs'.
'baseUrl' option is set to '/tmp/test', using this value to resolve non-relative module name 'URI'.
Resolving module name 'URI' relative to base url '/tmp/test' - '/tmp/test/URI'.
Loading module as file / folder, candidate module location '/tmp/test/URI', target file type 'TypeScript'.
File '/tmp/test/URI.ts' does not exist.
File '/tmp/test/URI.tsx' does not exist.
...
File '/tmp/test/node_modules/URI.js' does not exist.
File '/tmp/test/node_modules/URI.jsx' does not exist.
Directory '/tmp/node_modules' does not exist, skipping all lookups in it.
Directory '/node_modules' does not exist, skipping all lookups in it.
======== Module name 'URI' was not resolved. ========
...

But in any case, setting compilerOptions.allowSyntheticDefaultImports to true as suggested solves the issue, so there is nothing directly for etebase to fix here.

My improvement suggestions:

tasn commented 3 years ago

Sorry for the slow reply, it's been hectic! I merged the branch. README: would you mind opening a PR with the suggested changes? Changelog & tags: I know, haha. We've been lazy about it with the js lib (we maintain in for the other libs).

hlovdal commented 3 years ago

Commands to tag all releases are:

git tag v0.42.0 71fd7ce
git tag v0.41.1 4baf1e8
git tag v0.41.0 3f04589
git tag v0.40.1 05dd5fe
git tag v0.40.0 48d2b57
git tag v0.30.0 08cbf84
git tag v0.21.0 3d60ed0
git tag v0.20.0 403da3e
git tag v0.14.0 29d9019
git tag v0.13.2 19bd54d
git tag v0.13.1 a1d256b
git tag v0.13.0 09126a3
git tag v0.12.0 c92eb51
git tag v0.11.0 b6c2ba3
git tag v0.10.4 a4900fa
git tag v0.10.3 a355314
git tag v0.10.2 404b863
git tag v0.10.1 a9063fc
git tag v0.10.0 f019571
git tag v0.9.5 88db2d8
git tag v0.9.4 39c7c2e
git tag v0.9.3 8abc9a6
git tag v0.9.2 e9c9973
git tag v0.9.1 7d79346
git tag v0.9.0 1e0cd72
git tag v0.8.0 90b87f3
git tag v0.7.4 bd05207
git tag v0.7.3 75c682d
git tag v0.7.2 50da8f1
git tag v0.7.1 ced6d54
git tag v0.7.0 2c5f76c
git tag v0.6.1 80269b5
git tag v0.6.0 6b290ff
git tag v0.5.0 083fc6d
git tag v0.4.0 9130dbf
git tag v0.3.1 c47baa6
git tag v0.3.0 101f712
git tag v0.2.0 d406ead
git tag v0.1.5 22b39fd
git tag v0.1.4 b910ba6
git tag v0.1.3 acf3758
git tag v0.1.2 0b513dd
git tag v0.1.1 086636a
git tag v0.1.0-beta.6 3f1a85b
git tag v0.1.0-beta.5 5f26b19
git tag v0.1.0-beta.4 7c8faa2
git tag v0.1.0-beta.3 856c385
git tag v0.1.0-beta.2 6804839
git tag v0.1.0-beta.1 950b329
git tag v0.1.0-beta.0 0ada955
git tag v0.1.0 5eab235

(it was trivial to make this list when I already extracted versions for the changelog)

tasn commented 3 years ago

The problem is that these are non-annotated tags. I can use this list however to write a script to create annotated ones. I'll try to get to it soon.