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

fix: fixes typescript and intellisense #2562

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

groozin
Copy link

@groozin groozin commented Oct 19, 2023

Summary

Fixes #2548

Both intellisense and typescript tests were broken on master. Though the issue mentions webstorm as the editor I had the same with vscode.

Converted the index.ts into index.js with CJS syntax as per eslint requirement. Also fixed the typescript tests and added them to npm run test:full script - now that they are working.

Test plan

For testing I ran npm run build

 ~/r/exceljs | fix/2548/fix..intellisense                                                                                                                51s | 20.8.0 node | 2.7.5 rb
> npm run build

> exceljs@4.3.0 build
> grunt build

Running "babel:dist" (babel) task

Running "browserify:bare" (browserify) task
>> Bundle ./dist/exceljs.bare.js created.

Running "browserify:bundle" (browserify) task
>> Bundle ./dist/exceljs.js created.

Running "browserify:spec" (browserify) task
>> Bundle ./build/web/exceljs.spec.js created.

Running "terser:dist" (terser) task
>> 1 grunt.util.pluralize(createdFiles, 'file/files') created.

Running "terser:bare" (terser) task
>> 1 grunt.util.pluralize(createdFiles, 'file/files') created.

Running "exorcise:bundle" (exorcise) task
Exorcising source map from ./dist/exceljs.js
Exorcising source map from ./dist/exceljs.bare.js

Running "copy:dist" (copy) task
Created 20 directories, copied 332 files

Done.

Then npm run test (the output is really big so I omit it here).

Also to check intellisense working and to make sure that different module approaches are still working I created 3 files: one ESM, one CJS and one TS. I then use npm install from the local source code of my branch to have the changes in. And both checked the intellisens and ran the code in the files to see if the lib is working.

See below:
image

Related to source code (for typings update)

Copy link
Member

@Siemienik Siemienik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your engagement, that's really great to have opportunity review your PR.
I little misunderstood the reason of this PR, as all works good for my projects.

import { expect } from 'chai';
import ExcelJS from '../../index';
import { expect } from "chai";
import * as ExcelJS from "../../index";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually breaking compatibility change, I don't see reason of doing that now. Also importing everything from exceljs works great for my projects.
What kind of error have you got?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be glad for sharing me your test repo from screenshots.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey sorry missed this message, will send you the repo later today

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed the repo here: https://github.com/groozin/exceljs-test
That being said - using the vscode version

Version: 1.85.1 (Universal)
Commit: 0ee08df0cf4527e40edc9aa28f4b5bd38bbff2b2
Date: 2023-12-13T09:48:06.308Z
Electron: 25.9.7
ElectronBuildId: 25551756
Chromium: 114.0.5735.289
Node.js: 18.15.0
V8: 11.4.183.29-electron.0
OS: Darwin x64 23.1.0

and node 18 I cant reproduce anymore. Still using the version 4.30 of the exceljs.
Maybe the recent vscode versions fixed the problem.
But just in case I uploaded the repo I used to test.

@alexandre-embie
Copy link

I actually got the same exact problem and this PR fixed it. To ensure proper development, I had to specifiy
"exceljs": "github:groozin/exceljs#fix/2548/fix-typescript-and-intellisense"
in my package.json in the devDependencies

Please fix this.

@marisuxma
Copy link

Any update @Siemienik @groozin ?

@groozin
Copy link
Author

groozin commented Jan 1, 2024

@marisuxma @alexandre-embie I tried both versions 4.3.0 and 4.4.0 and can't reproduce with the latest vscode. Meaning all is working well for me now. Lets see how to progress here.

@alexandre-embie
Copy link

I'm using webstorm. I think that's where the problem is from, but your package is the only one I have that has this problem so there has to be something setup wrong

@clecherbauer
Copy link

Hi, i have this issue too

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.

[BUG] Typescript typings don't work
5 participants