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

Types (.d.ts) in npm package #463

Merged

Conversation

pmstss
Copy link
Contributor

@pmstss pmstss commented Mar 4, 2019

Pull Request Description:
I've enabled d.ts generation in tsconfig.json and added typings property into package.json.

Motivation:
Utilize types power in pretty complex jsPDF-AutoTable config without import of source files. Currently in project I have to had something like:

import { Cell, Table } from 'jspdf-autotable/src/models';

which is generally bad practice and causes issues with tsc.

@pmstss pmstss changed the title auxiliary index.ts with all exports; types reference in package.json;… Types (.d.ts) in npm package Mar 4, 2019
@simonbengtsson
Copy link
Owner

Awesome! Three thoughts:

Can a typescript example be added? I guess it could be something similar to the requirejs and webpack examples.

I'm a little hesitant to pollute the dist folder with that many new files. Is it possible to merge all of the definitions into one file?

I have not verified it, but it looks like some internal types are exposed. Is this required or can they be hidden somehow?

@pmstss
Copy link
Contributor Author

pmstss commented Mar 5, 2019

Thanks for feedback!

Can a typescript example be added? I guess it could be something similar to the requirejs and webpack examples.

Ok, I will try to prepare it today or tomorrow.

I'm a little hesitant to pollute the dist folder with that many new files. Is it possible to merge all of the definitions into one file?

Yes this is what I also don't like much and tried to avoid in previous PR (#462), but after playing with various options still got invalid either from the point of syntax (with outputAsModuleFolder option) or usage (without it).
Alternative option can be extracting types to separate file or files - and distributing only them.

But even more important from the point of space/bandwidth pollution is that all files and folders other than 'dist' are redundant for npm distribution. Now node_modules/jspdf-autotable is about 5MB (proof: 1.4MB tgz link to npm registry) and with dist only (even with new .d.ts files) could be ...only 100KB. I will prepare another PR if you don't mind.

I have not verified it, but it looks like some internal types are exposed. Is this required or can they be hidden somehow?

Yes, now everything "exportable" is exported through newly added index.ts with wildcards export statements inside (export * from ...). Limited set of entities can be provided there instead of wildcards. I'm not pretty sure what you are referencing as internal types, but e. g. in my project I have used only these ones: Table, Cell, CellHookData, ContentConfig and UserOptions - please keep them exported)

@simonbengtsson
Copy link
Owner

simonbengtsson commented Mar 5, 2019

Thanks for letting me know about the lib size! Reduced it too ~100kb now (only dist folder) in v3.0.13.

Hmm, how about in a separate types directory? Maybe we can generate it as apart of the build process and then remove it after publish?

And after looking into it a bit more my concern over exposing internal types was probably invalid since the user will never interact with them.

@pmstss
Copy link
Contributor Author

pmstss commented Mar 5, 2019

I've added typescript+nodejs sample (examples/typescript-node). npm install in that folder should trigger postinstall hook that will compile main.ts and run it producing examples/typescript-node/output.pdf.

Hmm, how about in a separate types directory? Maybe we can generate it as a part of the build process...

Yes. highly possible that d.ts files could be "moved" from ./dist to ./dist/types by configuring some tsc options. Will it be suitable?

...then remove it after publish?

Not sure what do you mean by "after publish", but for npm package all ./dist/*.d.ts files are required because current ./dist/index.d.ts contains references to them; and after unsuccessful tries with dts-bundle it's not clear how to "embed" them.
I need a bit more time to find elegant solution...

@pmstss
Copy link
Contributor Author

pmstss commented Mar 12, 2019

I'm a little hesitant to pollute the dist folder with that many new files. Is it possible to merge all of the definitions into one file?

I have added dts-bundle-generator dependency and generate-dts npm script to produce single index.d.ts file in ./dist folder.

PR summary:

@stale stale bot added the wontfix label Mar 23, 2019
@stale stale bot closed this Mar 23, 2019
@stale stale bot removed the wontfix label Mar 23, 2019
Repository owner deleted a comment from stale bot Mar 23, 2019
@simonbengtsson
Copy link
Owner

Not sure if I'm doing something wrong here, but when I run npm start in the added example I get an ReferenceError: URL is not defined error. Something you have seen?

@simonbengtsson
Copy link
Owner

Plus, it would be super nice if it was possible to implement that you would get jsPDF.autoTable() automatically typed after doing `import 'jspdf-autotable'. Do you think this would be possible?

@simonbengtsson
Copy link
Owner

simonbengtsson commented Mar 23, 2019

After looking into it a bit it seems impossible to extend the published jsPDF types with a autoTable function (without modifying them somehow). The next best thing I can think of is the below. What do you think?

  1. In main.ts a export type autoTable = (arg: UserOptions) => void or similar is added.
  2. We use the dts-bundle-generator to generate types similar to this PR.
  3. Usage would then be something like this:
import * as jsPDF from 'jspdf';
import 'jspdf-autotable';
type AutoTable = import('jspdf-autotable').autoTable;

const doc = new jsPDF();
doc.text("Text", 100, 100);
((doc as any).autoTable as AutoTable)({
    html: '#sdjf',
    showHead: 'everyPage'
});

@simonbengtsson simonbengtsson merged commit 6a18848 into simonbengtsson:master Mar 24, 2019
@pmstss
Copy link
Contributor Author

pmstss commented Mar 26, 2019

when I run npm start in the added example I get an ReferenceError: URL is not defined error. Something you have seen?

I've tried to run that example from scratch (with node_modules removing and npm install) and do not get this error. Not sure could it be node version or OS dependent (using node 11.9.0 on Windows 10 x64). Stacktrace would be helpful for further guesses.

Regarding to autoTable() function types would it be more straightforward to add definitions to
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/jspdf/index.d.ts (types for some other plugins are already there)? I will think about alternative ways a bit more and will let you know in case of any worthy ideas.

@simonbengtsson
Copy link
Owner

The plugins included in DefinitelyTypes are included the jspdf dist bundle which this plugin is not. It seems it should be possible to extend a class with new props, but with the types on DefinitelyTyped can't find a way.

https://stackoverflow.com/questions/55328516/augment-a-class-with-new-properties-exported-with-export-myclass

@crazyx13th
Copy link

I found one example
@types/webpack-dev-server extends a part of @types/webpack
maybe this helps :-)
thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants