interledgerjs / ilp-plugin-btp

This has been moved to the monorepo https://github.com/interledgerjs/interledgerjs
8 stars 7 forks source link

Use build directory for js and avoid tsc on npm install #45

Closed karzak closed 5 years ago

karzak commented 6 years ago

When building plugins, differences in tsconfig (particularly use of strict) can cause installing btp to fail on otherwise successful builds. With this commit, npm publish will only include build files and avoid tsc when installing btp from npm.

karzak commented 6 years ago

@adrianhopebailie @sharafian @sentientwaffle Can I get an ack or nack on this? The use of 'tsc' in post-install seems like an anti-pattern I would like to get away from in all major ILP dependencies, but if there are strong objections, I'm open to hearing them.

sentientwaffle commented 6 years ago

I just npm-installed ilp-plugin-btp and it didn't run tsc on post-install. I think it only does that when installing devDependencies, which I think is ok.

karzak commented 6 years ago

Sorry, this was poorly described. The issue is as follows:

If I'm writing a package that has ilp-plugin-btp as a dependency and I have strict: true set in my tsconfig.json file. I run tsc --project . on my project and it fails with

/node_modules/ilp-plugin-btp/src/ws-reconnect.ts:61:11 - error TS2564: Property '_connected' has no initializer and is not definitely assigned in the constructor.

61   private _connected: boolean

This is because tsc is called in the prepare script in package.json (not post-install, my bad), and then uses the compiler options from my project, not those found in btp.

In general, I would like to avoid publishing the typescript files to npm.

roblav96 commented 6 years ago

In general, I would like to avoid publishing the typescript files to npm.

Agreed, source files are for source control (github), not a published package.

publishing-typescript-projects-with-npm When publishing a TypeScript project on NPM, run a prepublish script to create the JavaScript and TypeScript definition files and publish those files instead of your TypeScript files.

Also, prepublish and prepare: DEPRECATION NOTE

adrianhopebailie commented 6 years ago

@karzak @kincaidoneil - I was using ilp-logger as a way to try an find a Typescript library structure that we all agree on so we can get some consistency across projects.

That seems to have consensus now (short a pretty minor question that is outstanding) so why don't you replicate that structure here?

https://github.com/interledgerjs/ilp-logger/pull/11

karzak commented 6 years ago

@adrianhopebailie Thanks for pointing to that, I hadn't seen it. I'll take a look, raise any concerns there, and revise here.

sentientwaffle commented 6 years ago

Since the source maps are published, won't stack traces refer to nonexistent .ts files if they aren't published with the package?

kincaidoneil commented 6 years ago

@sentientwaffle Yeah, .ts files in the stack traces won't actually exist, but since debuggers (e.g. Chrome devtools, VS Code) are able to generate them from the source maps, I don't think it's an issue. e.g. if you want to debug the module itself and modify those source files, you'd probably want to clone the module anyways?

adrianhopebailie commented 6 years ago

@sentientwaffle WDYT? Can we merge this?

sentientwaffle commented 6 years ago

Mostly LGTM. I'm still not convinced that the sources should be excluded from the npm package. Chrome devtools & VS Code may work w/o them, but if the stack trace includes a file with a line number, I want to be able to open that file at that line number in an editor (e.g. vim) without special tooling to interpret it.

adrianhopebailie commented 6 years ago

@sentientwaffle is it possible to include the sources in such a way that they aren't picked up when trying to build a dependant project?

That's the issue I am still having with ilp-connector and it's a pain.

Is there a way to setup tsconfig in a project that depends on ilp-connector so it doesn't try to build ilp-connector?

sentientwaffle commented 6 years ago

Have you tried adding "node_modules" to the tsconfig.json's exclude list?

adrianhopebailie commented 6 years ago

Have you tried adding "node_modules" to the tsconfig.json's exclude list?

Yep. I need to dig into how tsc uses those settings. It seems like setting rootDir (or something) impacts whether it looks at excludes.

This is my config:

{
  "compilerOptions": {
    "target": "es2017",
    "module": "commonjs",
    "declaration": true,
    "noImplicitAny": true,
    "removeComments": true,
    "moduleResolution": "node",
    "sourceMap": true,
    "inlineSources": true,
    "strictNullChecks": true,
    "suppressImplicitAnyIndexErrors": true,
    "outDir": "build",
    "rootDir": "src",
  }, 
  "exclude": [
    "node_modules",
    "build/**/*"
  ]
}