nicojs / typed-inject

Type safe dependency injection for TypeScript
Apache License 2.0
454 stars 23 forks source link

Suggestion: Avoid adding source maps paths to published files #16

Closed felladrin closed 4 years ago

felladrin commented 4 years ago

Currently, files published to NPM have source maps defined, pointing to .map files that were not published.

image

image

This leads to warnings like this when using Parcel Bundler:

image

To avoid this, the build script on package.json could be changed to:

"build": "tsc -b --sourceMap false"

What do you think?

nicojs commented 4 years ago

Hi @felladrin 👋

I didn't use parcel with typed-inject yet, so thanks for opening this issue! 🌹

When running typescript in --build mode, it doesn't allow you to override compiler options:

error TS5072: Unknown build option '--sourceMap'.

Another solution might be to bundle the .map files and .ts source files with this package. I just hope parcel won't recompile them. What are you're thoughts on this?

felladrin commented 4 years ago

Hey @nicojs!

Thanks for checking! I didn't know about this restriction on build mode.

Ah, I like the idea of publishing also the TS files! I also think it could be encouraged to use the TS files directly when users are working on a TS project, so the files will be compiled using the same config of their project. For example, in this lib I left instructions for people who want to include the TS source files instead of using the transpiled files.

Would be great to have this possibility!

And I confirm that if the TS files are included, Parcel won't complain about the maps :)

nicojs commented 4 years ago

Ah, I like the idea of publishing also the TS files! I also think it could be encouraged to use the TS files directly when users are working on a TS project

Sorry, but this is a definite NO GO. I feel pretty strongly about this. The problem is: there is not one 'typescript'. A number of issues arise when you force people to compile your code (feel free to replace typed-inject with any dependency you might have):

Thanks for testing out Parcel. I'll check if typescript will compile typed-inject when it is a dependency of a "normal" typescript project. If not, I'll add it 👍

EDIT: As a nail in the coffin for compiling with different projects:

image

So typescript is compiling typed-inject if it is a dependency and comes with typescript files. I'll try some more stuff and come back to you.

felladrin commented 4 years ago

Ah, those are pretty good arguments! Made me also reconsider exposing TS on my lib. Alright, I'll just wait for the update for Parcel. Thank you!

nicojs commented 4 years ago

Well, it's apparently OK to publish the TS sources, just be sure to not compile to the same directory.

As you can see in the PR, I now direct TS output files to dist (which is a good practice either way).

I think this is how it works. When you import from a dependency, for example import { rootInjector } from 'typed-inject', TypeScript will resolve the same way as nodejs does (at least, with --moduleResolution node). It will look for typed-inject in your node_modules and it will find it. Next, it will read the package.json to see if there is a typing field. There isn't one, so it will look for a main field instead. There is one that points to dist/src/index.js. So it will search for a dist/src/index.ts file to compile. There isn't one, so it looks for dist/src/index.d.ts as a fallback. Tada 🎉.

The map files are now also published, so it will allow them to resolve the typescript source files as well.

nicojs commented 4 years ago

Please use version 2.2.1 🎉