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

feat: allows import core version in typescript #1713

Closed
wants to merge 4 commits into from
Closed

feat: allows import core version in typescript #1713

wants to merge 4 commits into from

Conversation

anion155
Copy link

Description

Allows typescript users to import core version of paper as import paper from 'paper/dist/paper-core';

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the JSHint rules (npm run jshint passes)

@anion155
Copy link
Author

anion155 commented Oct 3, 2019

@sasensi

@sasensi
Copy link
Contributor

sasensi commented Oct 4, 2019

Hi, thank you for your contribution.
Could you elaborate a bit more about what this change does (how it works, in what case that would be usefull...) ?
And could you imagine a test that would validate this usage ?
I already use a TypeScript file compilation as a test so maybe this could be based on the same principle ?

@anion155
Copy link
Author

anion155 commented Oct 4, 2019

Paper by default export paper-full which is has acorn, PaperScript, and all this beautiful staff, which is not always really needed (see paperjs/paper.js#which-version-to-use). I needed a way to import core version of the lib, but I did loose typings.

This file will allow to import paper-core with typings as default. Which is not perfect solution, but it's works. There is a limitation on re-exporting modules with export = (see microsoft/TypeScript#4336).

Tests should be exacly like in typescript-definition-test.ts but without PaperScript staff I suppose, and sadly with only default import. I think.

If you think this solution is worth it, I will add tests.

@anion155
Copy link
Author

anion155 commented Oct 4, 2019

Also, can re-export all named exports manually, like:

import paper, { Color } from 'paper';
export default paper;
export { Color };

@sasensi
Copy link
Contributor

sasensi commented Oct 4, 2019

I think that the best way to figure out if this is needed would be to set up a project reproducing the issue (using paper-core in a TypeScript project but losing typings) and see if this is the best way to solve it or if there is another workaround.
This StackBlitz can serve as a starting point.
Do you think that you could do that (maybe based on your own use case) ?

@anion155
Copy link
Author

anion155 commented Oct 4, 2019

Ok, here it is. As you can see there is no typings for core version.

@anion155
Copy link
Author

anion155 commented Oct 4, 2019

@sasensi I did found out if I add to paper.d.ts declaring of module paper/dist/paper-core with export = paper;, and in file paper/dist/paper-core.d.ts just do import 'paper'; everything work great. But there is still exports of PaperScript, which is not available in paper-core version.

@sasensi
Copy link
Contributor

sasensi commented Oct 5, 2019

@anion155, thank you for the reproduction case, it is much clearer for me now.
So yes, I would say that this is a valid use case and we should be able to find a solution for it.

Your last solution seems cleaner than your first proposal, but indeed, PaperScript class should not be available in paper-core module.
I guess that we could split the typescript declaration into two parts: the paper-core base on one side and the paper-full extra features on the other (I only see PaperScript class here for the moment).
Then, we would have 2 different module exports, as you propose.

This is the first time that I face this pattern (multiple possible exports of a library) so I don't know yet how to achieve that.
But I guess that we should be able to find another library that uses this pattern and see how they handle it.

@sasensi
Copy link
Contributor

sasensi commented Oct 5, 2019

@anion155, I think that I found a solution based on my idea of splitting the declaration in 2 parts.
Here is the StackBlitz.
I think that it resolves your issue and it should still work in normal cases.
The only thing is that the declaration generation needs to be completely refactored but that's something that I can handle (as I wrote it) if the solution seems good to you too ?

@anion155
Copy link
Author

anion155 commented Oct 5, 2019

Just a tip. Do not forget to actually add file paper-core.d.ts, 'cause if you actually core module without reference-thing, or import full version somewhere, you would get error Cannot find declarations for ....

sasensi added a commit to sasensi/paper.js that referenced this pull request Oct 6, 2019
Typings were missing when importing paper core version with:
`import * as paper from 'paper/dist/paper-core'` syntax.
This changes the generated TypeScript definition so that it exports two
modules: `paper` and `paper/dist/paper-core`. In the same logic,
`paper-core.d.ts` file is added to make sure that the corresponding
definition is automatically loaded.
This also takes care of the fact that `PaperScript` class is not
available in paper core version, by removing it from the corresponding
TypeScript definition.
Finally, this also simplifies existing definition by directly exporting
a `PaperScope` instance as the module instead of duplicating all
`PaperScope` properties and methods on the module itself.
Closes paperjs#1713
@sasensi
Copy link
Contributor

sasensi commented Oct 6, 2019

@anion155, indeed, thanks for the tip.
I made a PR (#1716 ) that should solve this issue.
Please have a look at it and confirm that it works in your case too.

@anion155 anion155 closed this Oct 10, 2019
lehni pushed a commit that referenced this pull request Nov 7, 2019
* Allow paper core import in TypeScript

Typings were missing when importing paper core version with:
`import * as paper from 'paper/dist/paper-core'` syntax.
This changes the generated TypeScript definition so that it exports two
modules: `paper` and `paper/dist/paper-core`. In the same logic,
`paper-core.d.ts` file is added to make sure that the corresponding
definition is automatically loaded.
This also takes care of the fact that `PaperScript` class is not
available in paper core version, by removing it from the corresponding
TypeScript definition.
Finally, this also simplifies existing definition by directly exporting
a `PaperScope` instance as the module instead of duplicating all
`PaperScope` properties and methods on the module itself.
Closes #1713
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

2 participants