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

chore(build): solve initial .d.ts pain #128

Closed
martinmcwhorter opened this issue Feb 3, 2016 · 24 comments
Closed

chore(build): solve initial .d.ts pain #128

martinmcwhorter opened this issue Feb 3, 2016 · 24 comments
Assignees
Milestone

Comments

@martinmcwhorter
Copy link

When compiling an application that does an import into ng2-bootstrap the compiler will default to the .ts rather than the .d.ts -- which causes the compiler to unnecessarily compile the ng2-bootstrap module.

This could be solved by moving the .ts files into src/ within the published npm module.

@valorkin
Copy link
Member

valorkin commented Feb 3, 2016

known issue with configuration
exclude node_modules from ts compiling
should be fine
it depends of course on what you are using
but to be clear src\dist approach seems messy to me
plus IDE will be missing go to declaration functionality

@valorkin valorkin closed this as completed Feb 3, 2016
@valorkin
Copy link
Member

valorkin commented Feb 3, 2016

please update me if my advise helped
if not please provide gist\repo so I can help with configuration

@martinmcwhorter
Copy link
Author

Already had exclude for node_modules. Unfortunately because my code explicitly imports from 'ng2-bootstrap/ng2-bootstrap', that ng-bootstrap.ts and the .ts files it references are compiled.

This was becomes more of a pain in 1.0.2.beta-1 because moment is a dependency that won't be found by tsc compiling my application.

This is my existing tsconfig.

{
  "compilerOptions": {
    "target": "es5",
    "module": "system",
    "moduleResolution": "node",
    "sourceMap": true,
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "removeComments": false,
    "noImplicitAny": true,
    "outDir": "dev"
  },
  "exclude": [
      "node_modules"
  ]
}

Angular2 takes the approach of moving the .ts files to ts/ in its published module.

Rxjs does not provide the source .ts files in its published modle.

The Rxjs approach would be easier as it would only require an entry in the .npmignore -- but it seems like the Angular2 approach (ts/) is "more better".

If your open to the Angular2 approach I can add a publish task that will move the .ts before publishing to npm and make a pull request.

@valorkin
Copy link
Member

valorkin commented Feb 3, 2016

dirty hacks, both >.<
is there are no better solution?

@valorkin
Copy link
Member

valorkin commented Feb 3, 2016

and moment yes, I should somehow remove the initial pain with tsc

@valorkin
Copy link
Member

valorkin commented Feb 3, 2016

need to check date pipe, may be I will be able to reuse it somehow for data parsing
I want remove moment completely to be honest

@valorkin valorkin reopened this Feb 3, 2016
@valorkin valorkin changed the title Move .TS (not .d.ts) to src/ in published NPM package chore(build): solve initial .d.ts pain Feb 3, 2016
@valorkin
Copy link
Member

valorkin commented Feb 3, 2016

oh, btw
you are using "module": "system",
why not to use bundles?

@martinmcwhorter
Copy link
Author

I am not sure I understand your dislike to moving the source to ts/ in the npm package? Why does it seem like a hack? I am only asking for the sake of understanding.

It is natural that tsc would resolve references to the .ts rather than the .d.ts -- which it does. This makes it problematic to include both in the same path.

It seems there are three flavors of npm packages generated from typescript:

  • provide transpiled code and definitions
  • provide transpiled code, definition and source ts safely moved to another directory.
  • provide only ts source

The last option is a bad idea outright.

The first approach would be fine in most situations.

The middle approach is the most flexible. Developers could still step through the ts if they wished. The map files in the dev bundles can point to the node_modeles/ng2-bootstrap/ts/ path.

Moving the source to ts/ should be a npm publish step only. The directory structure of the source code shouldn't change.

@berhir
Copy link

berhir commented Feb 4, 2016

Yesterday I tried to add ng2-bootstrap to our project. But as soon as I imported anything from ng2-bootstrap/ng2-bootstrap the typescript compiler throw some errors. It took me some hours to figure out that the problem is that typescript definition files and source files are in the same folder in node_modules.

This is my tsconfig:

{
    "compilerOptions": {
        "target": "es5",
        "module": "system",
        "moduleResolution": "node",
        "sourceMap": true,
        "emitDecoratorMetadata": true,
        "experimentalDecorators": true,
        "removeComments": false,
        "noImplicitAny": false,
        "outDir": "wwwroot/app",
        "sourceRoot": "app"
    },
    "exclude": [
        "node_modules",
        "wwwroot"
    ]
}

When I run tsc I get this error:

error TS6059: File 'node_modules/ng2-bootstrap/components/accordion.ts' is not under 'rootDir' 'app'. 'rootDir' is expected to contain all source files.

This error repeats for all components.

When I remove the sourceRoot option from my tsconfig file, tsc shows no errors but it creates two separate folders in my output location.

wwwroot
|- app
  |- app                  (my compiled code, should be one level up)
  |- node_modules
    |- ng2-bootstrap      (compiled ng2-bootstrap, shouldn't be there)

For now I created a script that moves the ng2-bootstrap source ts files to a different location. But I hope you can move it to another location by default in the next release.

@valorkin
Copy link
Member

valorkin commented Feb 4, 2016

ok, I agree that as for now there are should be some hack for tsc,
but tsc it self should behave differently with 3rd party dependencies
and it should not be tricky to use or publish ts modules

@valorkin
Copy link
Member

valorkin commented Feb 4, 2016

in TS 1.8 there allowJs option microsoft/TypeScript#6725
need to try it
similar microsoft/TypeScript#6670

so my plan is:

  1. update ng2 to latest today
  2. update to tsc 1.8
  3. write a doc on integrating into seeds (mentioned in quick start)

@valorkin valorkin self-assigned this Feb 4, 2016
@martinmcwhorter
Copy link
Author

@valorkin if there is anything you would like a hand with let me know. I am willing to help sort this out.

The work done in the project is superb and much appreciated. Some of us who have been working in TypeScript and Angular for years are heartened to see such quality libraries as this one for Angular2. A lack of choice in UI controls would easily be a friction point in adopting Angular2. We all appreciate the work that has gone into this project.

I am not seeing the difficulty or hackishness of publishing segregated definitions and source to npm that you are.

It is not a perfect analogy, but it may be helpful to think of the transpiled bundles as your object code and the d.ts definition files as the header files that provide the APIs, interfaces, to your bundles.

So those the definitions and the bundles should be the "meat" of the npm package. Providing source in the package is fine -- but not essential. That is what github is for.

Indeed going to the definition of a library is often useful. Stepping through a debugger and seeing source of a library is useful as well. That said, neither of these should be the default behavior as it violates the OOD principle of encapsulation. A library by definition should only expose a well designed interface (.d.ts) and not its implementation (.ts). Again -- there should be easy ways to see the source.

For example change:
import {Alert} from 'ng2-bootstrap/ng2-bootstrap'; to import {Alert} from 'ng2-bootstrap/ts/ng2-bootstrap'; to be able to step into the implementation.

@valorkin
Copy link
Member

valorkin commented Feb 4, 2016

yeap, may be you are right
but I want to believe that it will be possible to not do it in future
when tsc will became more mature and ready for stuff like node_modules stuff

@valorkin
Copy link
Member

valorkin commented Feb 4, 2016

Thanks for kind words :)
main problem I see, that any tests written for original code is not testing modified source
but we will get to it later
I want to be able to publish each component separately
to easily reuse them in ng2-table or ng2-file-upload for example

@valorkin
Copy link
Member

valorkin commented Feb 4, 2016

and to be honest,
I am not even 15% complete with this lib
so much left to do :)
https://github.com/valor-software/ng2-plans

@mirkonasato
Copy link

Stumbled into the same issue.

error TS6059: File 'node_modules/ng2-bootstrap/components/accordion.ts' is not under 'rootDir' 'src'. 'rootDir' is expected to contain all source files.

As a workaround I just removed all *.ts files except the *.d.ts ones for now, with

find node_modules/ng2-bootstrap -name '*.ts' | grep -v '\.d\.ts$' | xargs rm

@valorkin
Copy link
Member

valorkin commented Feb 7, 2016

how do you compile it?
without .ts files it did not worked for me with webpack

@valorkin
Copy link
Member

valorkin commented Feb 7, 2016

@martinmcwhorter do you have

"exclude": [
    "node_modules"
  ],

in tsconfig?

@martinmcwhorter
Copy link
Author

@valorkin yes. I have posted my tsconfig earlier in the thread.

@valorkin
Copy link
Member

valorkin commented Feb 8, 2016

trying to understand why .js only version fails for webpack :)

@Lanayx
Copy link

Lanayx commented Feb 24, 2016

+1. Annoying node_modules folder appears in the outDir location for ng2-bootsrap only.

@valorkin
Copy link
Member

yea, it is really annoying, came with ts 1.7.5

@SIGAN
Copy link

SIGAN commented Mar 17, 2016

@valorkin Hi, this is very important, because I have to remove *.ts files via postinstall, which is not correct, but at least it works

@drallgood
Copy link

By the way. This is causing gulp-typescript to generate into the wrong destination folder.
I don't know why, but it generates into dist/app/ instead of dist/ the moment I start using this project.

@valorkin valorkin added this to the beta.12 milestone Mar 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants