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

Solution for #72 + Add type definitions for Text(D)Encoder #86

Merged
merged 7 commits into from
Sep 15, 2022

Conversation

HeavenVolkoff
Copy link
Contributor

@HeavenVolkoff HeavenVolkoff commented Sep 13, 2022

Hello,

This PR presents a solution for #72. It adds a notice to the README informing users that it is recommended to edit their tsconfig.json file for GJS projects. Besides, logic was built to check, during runtime, if the project has the DOM lib in its tsconfig.json and if that is the case ask the user if he wants to skip generating any conflicting type.

Also, included in this PR are the definitions for the Text(D)Encoder interfaces added in GJS 1.69.2:
https://gitlab.gnome.org/GNOME/gjs/-/blob/572f8e8c/NEWS#L384-393
https://gitlab.gnome.org/GNOME/gjs/-/blob/master/doc/Encoding.md
https://gitlab.gnome.org/GNOME/gjs/-/blob/master/modules/esm/_encoding/encoding.js

 - Improve documentation for Text(D)Encoder
 - Add an explicit type listing all available encoding suported by Gjs TextDecoder
…fig file

 - Warn gjs users if the project includes DOM lib
 - Don't generate conflicting types in Gjs global when project includes DOM lib
 - Add notice recommending modifing tsconfig.json for GJS projects
@HeavenVolkoff
Copy link
Contributor Author

Will check why the tests are failing

 - Fix a bug in readTsJsConfig that caused it to not resolve relative paths parents
 - Change typescript to a runtime dependency due to the usage of its internal JSON parser
@HeavenVolkoff
Copy link
Contributor Author

HeavenVolkoff commented Sep 15, 2022

So, typescript has its own parser for tsconfig.json, and it seems to accept more than the pure JSON spec, because of this using JSON.parse is not possible as it can fail with valid tsconfig.json. My solution was to simply import the typescript compiler's own parser and use it directly. The consequence is that this requires typescript to be a runtime dependency, IMHO I don't think this will have much of an impact for end users, as this tools will probably be a devDependency on project using it and most of them will probably also use typescript. However, if you don't agree with this, I can look for alternative parser that may offer a better compatibility with tsconfig.json, json5 seems to be a good alternative.

 outddir is not required to exists when the tsconfig.json parse logic executes
@JumpLink
Copy link
Collaborator

JumpLink commented Sep 15, 2022

However, if you don't agree with this, I can look for alternative parser that may offer a better compatibility with tsconfig.json, json5 seems to be a good alternative.

@HeavenVolkoff Another alternative is jsonc-parser which is from Microsoft, so maybe this package is also used internally in TypeScript?

I have used pkg-types in another project which can read the package.json and the tsconfig.json, is using jsonc-parser for parsing and has good TypeScript Type Definitions. What do you think about it?

P.S. Today is my birthday and I just saw your pull requests today, which is a great birthday present, thank you! :)

Edit
I just realized that the tsconfig.json can also inherit from other config files using extend, to parse this should work better with your current solution so I will merge that now :)

@JumpLink JumpLink merged commit b356277 into gjsify:master Sep 15, 2022
@HeavenVolkoff
Copy link
Contributor Author

@HeavenVolkoff Another alternative is jsonc-parser which is from Microsoft, so maybe this package is also used internally in TypeScript?

Although jsonc-parser seems to be a good option, it isn't what Typescript uses internally, they actually implemented their own parser, because they seem to try parsing some broken JSON without failing. Here you can see their parser implementation:
https://github.com/microsoft/TypeScript/blob/main/src/compiler/parser.ts

Edit I just realized that the tsconfig.json can also inherit from other config files using extend, to parse this should work
better with your current solution so I will merge that now :)

Oh, that is a good point, I didn't consider it when I implemented this XD. Just went with the simplest solution. But, I would say it is the main advantage of using TS own parsing logic, even if they change format or break compatibility, it would have very little effect on this project.

P.S. Today is my birthday and I just saw your pull requests today, which is a great birthday present, thank you! :)

Hey man, Happy Birthday 🎂. Hope you enjoy your day.

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