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

May i use typescript rewrite fabricjs? #6763

Closed
boomyao opened this issue Dec 29, 2020 · 18 comments
Closed

May i use typescript rewrite fabricjs? #6763

boomyao opened this issue Dec 29, 2020 · 18 comments
Labels
stale Issue marked as stale by the stale bot

Comments

@boomyao
Copy link
Contributor

boomyao commented Dec 29, 2020

I want to fork repo and rewrite it by typescript.
if i do it great, can you merge this PR ?
@asturur

@asturur
Copy link
Member

asturur commented Dec 29, 2020

I m not interested in switching this repo to TS. For now a lot of people still just include the built file as it is. I do not want use time to add the tooling.
I m planning to move it to ES6 as soon as ie11 support gets removed anyway.

We could use an officiale type definition file anyway, possibly generated from JSDOC

@rwillett
Copy link

I'm using it with TypeScript, Electron and Angular 10 now. For my naive use it seems OK.

Rob

@stale
Copy link

stale bot commented Jan 12, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Jan 12, 2021
@stale stale bot closed this as completed Jan 19, 2021
@SLKnutson
Copy link
Contributor

Typescript could be a big win for a complicated project like this. It is a good way to avoid a lot of types of bugs. You can have it generate a build file downleveled to your desired es version.
For users, the nice thing about using a library that uses typescript directly instead of using DefinitelyTyped is that the typescript definition is always up to date and accurate. Right now, the fabricjs's type definitions are basically in disrepair and have inaccurate types for fabric 2.6.0.

@asturur
Copy link
Member

asturur commented Jan 20, 2021

would be good to have a script that generates the td file starting from jsdocs.

@BradleyHill
Copy link
Contributor

Compiling down to js will ALWAYS be accurate. Docs will ALWAYS have errors - that is just the nature of documentation...ever since the first caveman started documenting on cave walls. Guessing no persuasion here but just saying.

@asturur
Copy link
Member

asturur commented Jan 21, 2021

That is true, but still:

  • i won't rewrite fabric js in TypeScript because i do not use that technology and i would not feel comfortable with it
  • i do need to write jsdocs anyway and those have a better benefits ( of adding a description and notes )
  • definitely typed for fabricJS never worked, because no one is behind it

Having a .ts file generated from jsdocs would still be better than nothing.
Errors can be fixed.

@snebjorn
Copy link

I would appreciate an official type definition file.

@asturur
Copy link
Member

asturur commented Apr 14, 2021

i would too, we need someone to start it.

@snebjorn
Copy link

Hmm when creating an issue it says

Anything different than a bug report will be closed automatically.

So we can't create an issue to track progress/support

@asturur
Copy link
Member

asturur commented Apr 15, 2021

You can, i m soft with issues that make sense. First verify that there isn't one open already

@snebjorn
Copy link

Anything different than a bug report will be closed automatically.

Would have (and did) discouraged me from opening an issue like that. I would instead have tried to go to the DefinitelyTyped repo. But doing that means you basically have to open a PR yourself and fix it. And by then it's easier to just do quick and dirty cast in the code.

There is traffic on the @types/fabric package. That counts for something :)
https://www.npmjs.com/package/@types/fabric
https://github.com/DefinitelyTyped/DefinitelyTyped/issues?q=fabricjs

@asturur
Copy link
Member

asturur commented Apr 15, 2021

I actively discourage people from opening random issues for asking

  • how do i do this
  • i tried x but it doesn't work
  • i need this feature

There is the discussion section for that.

There are a lot of people using TS, i m not expert, i have no idea what does it take to maintain a .d.ts file, and i would love to be able to generate it from jsdocs

@snebjorn
Copy link

snebjorn commented Apr 16, 2021

I see. Haven't used github discussions yet. So not familiar with when to use it :)

i have no idea what does it take to maintain a .d.ts file

I haven't done it from a JS source. But according to https://www.typescriptlang.org/docs/handbook/declaration-files/dts-from-js.html it should be as easy as running

npx typescript src/**/*.js --declaration --allowJs --emitDeclarationOnly --outDir types

given that there's proper JSDoc


I just tried to run it and it generates an empty type definition. I think the issue is the JS is written in the IIFE style. Not sure if it can be made to work with IIFEs. I couldn't find anything.

Another option is to convert from IIFE to CommonJS or even better ES Modules.
Found this https://ui.dev/javascript-modules-iifes-commonjs-esmodules/

@ShaMan123
Copy link
Contributor

ShaMan123 commented May 27, 2021

+1
@asturur I think ts must be included in this project for it to enhance DEV UX, make it easier to use and make it even more popular than it already is.
The current definitions are lacking. I myself have a huge patch file for them and I don't bother to PR because it's too complicated IMO.
I think that if it would be part of the project when creating a PR you can enforce the PR to update the ts files.
I am sure the community will take part. I can say the least about myself!

@snebjorn
Copy link

There's an open discussion #7017 about how to get started on this

@Exlord
Copy link

Exlord commented Jun 9, 2021

Converting to ES6 would be great, then you can easily generate .d.ts files from jsdoc and make the TS people happy (including my self). And we can have treeshaking to reduce library size.
I am rewriting my whiteboard app from scratch just because this lib has a lot of stuff that I don't need in my new whiteboard.(I used fabric for the previews version).
I am dissecting your code and picking only the parts I need and moving to my code and converting to ts, and its a pain 😢

I used this config file to generate .d.ts files for my es6 modules project.

tsconfig.json

{
  // Change this to match your project
  "include": [
    "src/**/*"
  ],
  "exclude": [
    "*.d.ts"
  ],
  "compilerOptions": {
    "module": "CommonJS",
    // Tells TypeScript to read JS files, as
    // normally they are ignored as source files
    "allowJs": true,
    // Generate d.ts files
    "declaration": true,
    // This compiler run should
    // only output d.ts files
    "emitDeclarationOnly": true,
    // Types should go into this directory.
    // Removing this would place the .d.ts files
    // next to the .js files
    "rootDir": "src",
    "outDir": "types"
  }
}

install ts globally first and then just type tsc in the console

@asturur
Copy link
Member

asturur commented Jun 13, 2021

But instead of picking up what you need, you could try a custom build or you could try to improve our code splitting mechanism.

Ultimately rewriting everything from es5 to es6 and named exports would utimately get to the point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issue marked as stale by the stale bot
Projects
None yet
Development

No branches or pull requests

8 participants