Skip to content

Do not publish .ts files on npmjs.org #162

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

Closed
victornoel opened this issue Mar 10, 2017 · 13 comments
Closed

Do not publish .ts files on npmjs.org #162

victornoel opened this issue Mar 10, 2017 · 13 comments
Labels

Comments

@victornoel
Copy link

Hi,

Apparently, angular2-notifications is published on npmjs.org with its ts files.
This is a bad practice according to (at least) angular-cli people (see angular/angular-cli#4675 (comment)).

It would be great if the ts files were removed in future releases and optionally for the latest release too :)

Thanks!

@flauc
Copy link
Owner

flauc commented Apr 9, 2017

Hi @victornoel

Sorry for responding so late.
I'm not sure about this one, why is this a bad practice?

Initially the src folder was excluded from the library but I got a lot of complaints about it.

@xealot
Copy link

xealot commented Apr 10, 2017

#111 explains why it breaks things. Unless something has changed it still holds true.

@flauc
Copy link
Owner

flauc commented Apr 10, 2017

Yes, but can't you exclude node_modules from your tsconfig and tslint?

@xealot
Copy link

xealot commented Apr 10, 2017

node_modules is excluded by default I believe. When you include things from node_modules and typescript sees a ".d.ts" file and a ".ts" file in the same place it will look at the ts file.

Do you know of any other TS libraries that ship ts source code with the NPM module? Perhaps I'm simply mistaken in all things.

@victornoel
Copy link
Author

I agree with @flauc that it is not a good enough reason: @xealot node_modules is NOT ignored by default, and you could/should ignore it.

I initially reported this issue because I understood from angular people that it was a bad practice (as you say @xealot, there is almost no npm module shipping their ts files). But I'm not exactly sure WHY it is a bad practice, I asked @filipesilva (angular/angular-cli#4675 (comment)) about it and I'm waiting for an answer.

Personally, I like the fact that ts files are present because I can browse the code of the dependencies I use, but it still seems it is not a practice usually followed…

@filipesilva
Copy link

filipesilva commented Apr 10, 2017

Copied from angular/angular-cli#4675 (comment)


@victornoel generally it's discouraged to ship TypeScript with third party libraries because it would require the consumer to replicate the complete build environment of the library. Not only typings, but potentially a specific version of ngc as well.

Publishing plain JavaScript with typings and meta data allows the consuming application to remain agnostic of the library's build environment.

@xealot
Copy link

xealot commented Apr 10, 2017

The point still remains that if you use the "rootDir" option in your code and it's pointed to something like src this will break projects.

Which means your library isn't compatible with a subset of projects that use this TS feature. Additionally, if I exclude node_modules will that still type-check the things I import from there?

@victornoel
Copy link
Author

@xealot you are right that there may be strange behaviour with respect to tslint typechecking if you remove node_modules! I didn't think of that :)

Thanks @filipesilva for the explanation.

@flauc
Copy link
Owner

flauc commented Apr 11, 2017

I'll remove the src folder from the published repo, with everything you pointed out I think there really isn't any point in having it in there.

Thank you for joining in and helping out 👍

@flauc flauc closed this as completed Apr 11, 2017
@flauc flauc reopened this Apr 11, 2017
@flauc
Copy link
Owner

flauc commented Apr 11, 2017

It looks like this actually breaks my build. Reverting back to having it in there for now.

@victornoel
Copy link
Author

Hi @flauc, I'm back on this.

I really think you should take this issue into account, here is another reason: microsoft/TypeScript#15363

Basically, when shipping ts files with your package, they get taken into account by tsc and can create problems .
Actually, yours is creating problem for me, I get the following error when using noImplicitAny:

ERROR in .../frontend/node_modules/angular2-notifications/src/simple-notifications/services/notifications.service.ts (51,72): Element implicitly has an 'any' type because type 'Icons' has no index signature.

@tplk
Copy link

tplk commented Jan 24, 2018

Also, Angular CLI incremental builds using ng serve seems to cause full angular2-notifications rebuild on any change.
Maybe I'm wrong but that's what I always see:
image

@flauc
Copy link
Owner

flauc commented Apr 2, 2018

The latest version no longer publishes .ts files

@flauc flauc closed this as completed Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants