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

Add basic TypeScript types #246

Merged
merged 5 commits into from
Sep 26, 2022
Merged

Conversation

9inpachi
Copy link
Contributor

@9inpachi 9inpachi commented Sep 1, 2022

Sorry, with #245 earlier today, in a hurray I ended up placing the browser field above the other module related fields. This is just to move that field lower and make the readability easier.

@linev
Copy link
Member

linev commented Sep 1, 2022

I just move today browser field to the end of package.json - see

d2f95f7

@9inpachi
Copy link
Contributor Author

9inpachi commented Sep 1, 2022

Ahan, cool. I didn't notice that commit. Thanks.

Then, I will keep this PR open to push some very basic types setup to make imports by name work in TypeScript. Because import { build } from 'jsroot/geom'; works but throws an error in TypeScript unless we use // @ts-ignore.

@9inpachi 9inpachi changed the title Reorder the browser field in package.json Draft: Add basic TypeScript types Sep 1, 2022
@9inpachi 9inpachi marked this pull request as draft September 1, 2022 18:43
@9inpachi 9inpachi force-pushed the fix/webpack-browser-1 branch from da72cc7 to bc92c33 Compare September 1, 2022 21:45
@9inpachi 9inpachi force-pushed the fix/webpack-browser-1 branch from 16a16fe to c430800 Compare September 1, 2022 22:41
@9inpachi 9inpachi force-pushed the fix/webpack-browser-1 branch from 0634dfb to 05f28a3 Compare September 1, 2022 22:46
@9inpachi 9inpachi changed the title Draft: Add basic TypeScript types Add basic TypeScript types Sep 1, 2022
@9inpachi 9inpachi marked this pull request as ready for review September 1, 2022 22:47
@9inpachi
Copy link
Contributor Author

9inpachi commented Sep 3, 2022

There are following ways of fixing the types error.

  1. Using node16 as the moduleResolution option in TypeScript. This is able to pick the exported modules from package.json's exports field but still shows a warning that the types are not there.
  2. Declaring TypeScript modules using "magically available interfaces"/"global types" like I did in this PR. This does not add any actual types but just declares the modules to direct TypeScript that it's a known module without any types.
  3. The most thorough way is to generate declaration files for the JavaScript code. This is done with JSDoc that's used to document types in JavaScript code. I gave it a try so it's in the commit history (can be reverted with 09f2d6c). It works but there seem to be some wrong types declared through JSDoc like object instead of Object3D for a three.js object which causes problems when we actually use the API interface.

I implemented the second option since it's the simplest without any side effects and without any need to change anything on the consumer side.

@9inpachi
Copy link
Contributor Author

9inpachi commented Sep 7, 2022

HSF/phoenix#497 needs this to pass the pipeline and complete the update.

@9inpachi
Copy link
Contributor Author

@linev could you please take a look at this?

@linev
Copy link
Member

linev commented Sep 21, 2022

Does it work for you?

If yes - I will be able to check it begin of next week.

@9inpachi
Copy link
Contributor Author

9inpachi commented Sep 21, 2022 via email

@linev linev merged commit 60c22ed into root-project:master Sep 26, 2022
@9inpachi
Copy link
Contributor Author

@linev it seems that the types are not added to the latest released version. Do you plan to make it a part of the major version release?

@linev
Copy link
Member

linev commented Dec 1, 2022

It will be in 7.3.0 version, which I plan to release shortly before next ROOT 6.28 release.
Up to now there is no concrete plans for it.
If necessary, I can backport changes to 7.2 branch and produce 7.2.2 tag.

@9inpachi
Copy link
Contributor Author

9inpachi commented Dec 1, 2022 via email

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.

2 participants