-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Guidance on shipping ts within node_modules #12358
Comments
Can you elaborate on the issue, i could not get the details reading through the issue. As for the recommendation, publish |
As Typescript user... Bobril-build currently depends on ts in node_modules, because it uses Typescript AST to recognize calls to translation module - modifying AST to produce message ids, or detecting used resources, or usages of images for creating of sprites. Yes in theory all this stuff could be parsed out of JS, but question would it have same robustness, error reporting, speed? I am also thinking about advanced minification (Closure style or better) which is impossible just from JS code. But upgrade to TS 2.0 was not all roses we had to fix some incompatibilities in our TS code, so I feel pain too. |
@Bobris I am afraid i do not understand the underlying issue. can you explain the problem or share a repro i can look at? |
Issue is that you fixing bugs in TypeScript compiler, making it stricter even without changing any TypeScript compiler options. This dir https://github.com/Bobris/Bobril/tree/7860bda9ac82520e2826ccbced38168eceb01cf6/package is compilable by TS 1.8.10 without errors, but not with TS 2.0.3. |
I see. but if you publish sources, would not your have to compile with matching version of the tools you use, and with matching compiler options (e.g. |
But I think with TS 2.0 even just D.TS would not save me, because I made it null strict, so D.TS now contains |
Distributing The OP was about recommendation. my recommendation is not to ship your |
Thanks for the response @mhegazy.
That's a fair point - I have a feeling that there might be more than one scenario being discussed in the thread. Speaking for myself this is not a scenario I look to use at all and I'm not totally certain I understand what each poster has asked for - I'm just trying to grapple with what is reasonable to support and what is not. Perhaps rather than me assuming wrongly I'll ask the posters on the original issue to pitch in with their expectations. So @ahelmel , @Tiedye, @g3r4n, @sumeet70 , @skysteve, @stevejhiggs could you pitch in with what you are hoping for here and lets see if we can identify if you're all after similar behaviours or if there is more than one issue going on here. @basarat - could you also pitch in with what you were planning to support and then abandoned? Just so we can gather everything in one place? Thanks! 🌷 |
sure, both me and @skysteve are actually working on the same project so I'll detail our exact scenario:
Now, we've actually worked around the problem for now as oddly our issues only occurred when "allowJs" was set to true and now we are typescript all the way through this is no longer needed. But with allowJs set to true we received errors along the lines of |
There are actually three issues. Personas : Publisher is the person publishing an npm module and Consumer is the person using it 🌹 noEmit for
|
Thanks everyone that has commented here; that's really helpful 🌹 @mhegazy, given you've said:
and also given the other scenarios outlined in this issue and @basarat's specific suggestion that, in the case where there is no emit, we execute the following code: if (outputText === null || outputText === undefined) {
const additionalGuidance = filePath.indexOf('node_modules') !== -1
? "You should not need to recompile .ts files in node_modules and should contact the package author to request them to use --declaration --outDir. More https://github.com/TypeStrong/ts-loader/issues/278"
: "";
throw new Error(`Typescript emitted no output for ${filePath}.${additionalGuidance}`);
} Which will prompt the user that having My gut feeling is that it is but I wanted to check with the TypeScript team before adding something that might propogate bad practice. (I don't think this will) |
sounds reasonable. I should note that we have issue #11946 tracking emitting node_modules files if they are listed explicitly in the |
Thanks @mhegazy - I'll plan to make the comment link back to this issue as this has more context. |
Hi @mhegazy, I'm just implementing the warning now.
I'm mindful of the change and wanted to make our warning configurable for TypeScript 2.1 depending on whether a file is mentioned in |
@stevejhiggs This is exactly what I am trying to do:
I thought I had it working, but now it is starting to breakdown. What workflow do you use to achieve this? |
@ricklove in the end we gave up. We got everything working for us but it became very obvious from reactions to this issue that it was not really a use case that was being considered and that we would always be fighting to tooling to achieve what we wanted. Right now we are using declaration: true along with outdir to create a dist folder that contains just the es5 js + d.ts files, the package.json is then copied to the dist file and the result then published as the final package. The resulting package is then consumable from anywhere. One improvement would be to set typescript to use es6 modules to allow tree shaking and we'll be doing this when we move our build pipeline to webpack 2. |
@johnnyreilly , remember the issue I raise? #7398 You can't really distribute ts file before that is handled properly. It is a systematic issue. If people start shipping ts files, the packages will be divided into two camps that are not compatible with each other. |
@stevejhiggs Thanks for explaining that. I'm doing a similar thing now. I am building to the "lib" folder and I have index.js at root pointed to the lib folder:
I still have my original source in the package under src, but it isn't used for anything right now. This seems to work pretty well. Also, thanks for letting me know about webpack 2 and tree shaking with es6. I wasn't aware of that possibility and I'll research it more. |
We use I have tried many different solutions, and the only one that works is using I have tried the One of the primary reason we like to have ts files in our |
There is no such limitation in TypeScript compiler API and I am saying it from position of author of bobril-build which is mostly TypeScript compiler and bundler focused on Bobril framework. We have hundreds of TypeScript only modules. Whole trick is to provide custom CompilerHost and correctly implement |
This is now conditionally supported in ts-loader: https://github.com/TypeStrong/ts-loader#allowtsinnodemodules-boolean-defaultfalse as of 4.3.0: https://github.com/TypeStrong/ts-loader/releases/tag/v4.3.0 Down to the hard work of @aelawson |
Is it possible for a library author to include TS source files (and sourcemaps pointing to them) in an NPM package, but to prevent those TS files from being re-compiled by default? If so, how to do it, and should this be the officially-recommended way to publish TS packages to NPM, as opposed to the "don't publish TS source at all" recommendation above? Or should there be some other solution that doesn't include source in the NPM package but at least points dev tools towards the original source, similar to how Source Server does for Windows and Visual Studio? (Personally I don't like this solution because it seems more fragile than publishing source, but wanted to throw it out as an option in case preventing compilation of TS source is too hard.) I'm asking because it seems like there are two separate questions discussed in this issue: I understand the debate above around (1), but why is (2) problematic? Seems like every library should make it easy for dev tools to show developers the original, pre-transpiled source code of that library. Otherwise how will developers know where to set breakpoints in library functions whose only NPM-published "source" is confusing transpiled-for-ES5 code? How can developers step into original library code in the debugger if libraries use features like async/await whose ES5 transpiled equivalents make linear debug-stepping an exercise in frustration? How will users browse the actual source code (for inspiration, for curiosity, or when investigating possible library bugs) of the libraries that they're using without having to switch over to GitHub to browse the TS source there? For example, here's the easy-to-understand, easy-to-debug TS source of the API.post() method from the
And here's the compiled version of the same code. Much more complicated and much less readable!
If you spend a few minutes with two monitors looking back and forth between TS and the transpiled code, you can probably figure out where to set breakpoints. But having to do this definitely slows down debugging and intimidates newbies. And if you thought setting breakpoints is hard, just try stepping into (in a debugger like VS Code) the library code, whose whose first action is to call an __awaiter helper function that was inserted by the transpiler. Holy crap the code below is confusing!
I know there's value in having developers understand that transpilation is happening under the hood, and to learn advanced debugging techniques for transpiled code. And somewhere there's a CS professor who's overjoyed that thousands of developers have to really, really, really learn how a state machine works. But it seems really unfair to novice developers to throw this much complexity in front of them if all they want to do is set a breakpoint in a library function or to step into a library function to understand why their code isn't working as expected. |
Such a pity that we can't use pure ts packages. And how d.ts files are affected by compilator settings that was used to compile them? If for example we use Anyway, wanted to say thanks for a great work of bringing static typing to js. Even when it creates problems it already solved alot more problems than created. |
Is there any working answer? I have absolutely the same situation as @toddbluhm and really wanna work with my linked modules for local development. @toddbluhm did you found a solution to this problem? |
@vashchukmaksim The solution was already given for this issue in the comment right after mine.
Now implementing both of those was quite a bit more work than I had the time for, so I didn't do that. Instead, I just stuck with what was working for me in my original comment |
Apologies for resurrecting this old issue, but I feel the release of npm 7 necessitates some discussion on this. Breaking up a repo into multiple smaller packages is a first class feature of node/npm via workspaces now. Being able to import pure Say I have 2 bundled web apps, each with their own package.json and watch script. Lets call them App A and App B. App A will run on modern browsers and can safely target es6. App B however needs to run on a legacy browser so can only target es5. (This is a very crude example, but there are plenty of other tsconfig options which could apply) Assume I am following the monorepo/workspace pattern and break up what was previously a monolith up into smaller, more reusable packages. These packages aren't published publicly, they don't need to work in a non-typescript consumer and they don't need to emit JS files. They exist purely to be reused across by other pure typescript packages in this hypothetical workspace. In order to make any common code packages compatible with both App A and App B, each needs to set up its own watch, Thus, if I have 1 common package, we will have 3 watches. 5 will result in 7, etc. I'd argue we shouldn't need anymore than 2 watches in this scenario. As @professional-human-being has explained, this just doesn't scale in terms of performance. Furthermore, compatibility will be a case of lowest common denominator, e.g. if App A can use latest JS features safely, but App B needs to target an older ES version, the common package will have to use App B's target for the compilation, bloating App A's build size unnecessarily. I realise this is a bit of a moan without any solutions offered. I also realise there are a lot of excellent reasons for why this hasn't been resolved yet. However I genuinely love Typescript and just want it to be as future proof as possible. In my opinion, this has the slight potential to hamstring the language as workspace tooling and adoption increases. |
As someone that ended up at this issue while figuring out how to deal with source maps in NPM packages, here is what I ended up with: tsc --sourceRoot "https://raw.githubusercontent.com/<user>/<repo>/$(git rev-parse HEAD)/src/" points the |
By default, ts-loader will not compile .ts files in node_modules. who can tell me what should i do |
# Contexto Ao tentar usar os presets de tipografia no `SGLearner`, tive um erro de TypeScript que poderia ser resolvido pela adição de um `as const` na declaração dos presets. No entanto, isso provocava um outro erro: ``` ERROR in ../node_modules/geekie-design-system/token-presets/basic.ts 30:2 Module parse failed: Unexpected token (30:2) You may need an appropriate loader to handle this file type. | letterSpacing: DSA_LETTER_SPACING_WIDE, | lineHeight: DSA_LINE_HEIGHT_XL, > } as const; ``` A sintaxe do TypeScript não era reconhecida dentro de um node module. Imagino que seria possível configurar para que fosse, mas aparentemente a prática mais comum é de transpilar os arquivos `.ts` para arquivos `.js` + declarações de tipos `.d.ts` (vide microsoft/TypeScript#12358). Dessa forma também é possível usar a lib em contextos não TS, o que devemos precisar por exemplo no Tamojunto. # Mudanças Fiz a configuração para que aconteça a transpilação de arquivos `.ts` para `.js` + `.d.ts`. Foi necessário: * Fazer com que o `tsc` seja rodado no `prepare` do `package.json`, para gerar os arquivos antes de a lib ser usada. * Determinar onde esses arquivos seriam gerados. O padrão seria de eles ficarem no mesmo local dos arquivos originais, mas isso deixaria o repositório desorganizado, então escolhemos deixá-los na pasta `build`. Para isso foi necessário renomear a `build` pré-existente, no PR #22. Discussão relacionada: https://geekie.slack.com/archives/C0547M0UDHR/p1692826665431029 * Alterar como era feito o import de tokens e presets. Antes eles eram da forma `import { ... } from 'geekie-design-system/tokens'`, mas a existência da pasta `build` exigiria que ficassem da forma `import { ... } from 'geekie-design-system/build/tokens'`. Acredito que esse não seria um bom caminho, pois exporia um detalhe de implementação (a existência da pasta `build`) para quem usa a lib. Optamos então por mudar os imports para serem na linha `import { ... } from 'geekie-design-system'`. Esse import na raiz é passível de ser configurado para olhar a pasta build, pelo campo `main` do `package.json`. A mudança do `as const` em si, para evitar os erros de TS nos usos dos presets, virá num outro PR (o #24). # Como testar * Rodar `yarn tsc` e ver que os arquivos `.js` e `.d.ts` são gerados na pasta `build` * Adicionar a lib com as mudanças num projeto (por exemplo o SGLearner) e ver que é possível importar os tokens e presets
Hello!
I'm raising an issue because I'm seeking guidance on whether TypeScript is intended to be used to ship
.ts
files insidenode_modules
. I'm asking as I'm one of the maintainers of ts-loader and this issue was raised on that very topic. It seems to have worked in the past (without any particular intention or effort on the part of ts-loader) but broke with TypeScript 2.0.@basarat took a look into it and ended up reaching this conclusion:
@basarat also suggested
I'm inclined to think this is a good idea and I was planning to implement this. Before I did so I wanted to see if this was in line with the way the TypeScript team intends the language to be used. I think this is probably encouraging good practice but I wondered if you could share a view on whether that's the case?
cc @jbrantly @HerringtonDarkholme
The text was updated successfully, but these errors were encountered: