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

Change JSON parser to JSON5 #63

Closed
Nike682631 opened this issue Apr 12, 2023 · 10 comments
Closed

Change JSON parser to JSON5 #63

Nike682631 opened this issue Apr 12, 2023 · 10 comments

Comments

@Nike682631
Copy link

Currently aliasHQ seems to be using a parser that does not recognizes comments in JSON. This can be problematic as many projects that might be allowing comments in their tsconfigs suddenly won't be able to do so if they use aliasHQ for parsing. This issue is to address that problem.

@IlCallo
Copy link

IlCallo commented Apr 13, 2023

For reference, a quick fix would be to change this line

: JSON.parse(text)

to use JSON5.parse(text)

@davestewart
Copy link
Owner

Hey folks, yes, completely agree and this is on the list!

For context, Alias HQ also needs to write to JSON, though this shouldn't prevent this from happening.

This is something I have wanted to fix for ages, so let me try and find some time over the weekend.

@IlCallo
Copy link

IlCallo commented Apr 17, 2023

Reading JSONC is pretty straightforward, while writing to JSONC format is tricky, so tricky that JSON5 mantainer just decided to not support it
Can we contribute with the small fix using JSON5 only for reading in the meantime?

After all, it seems that you would need to write to JSON only when initializing the project with isn't using new aliases, not when the user already setup its own aliases

@davestewart
Copy link
Owner

davestewart commented Apr 17, 2023

Yes please, go for it!

FWIW I may just use some kind of regular expression replacement for writing back to the file.

The config will only ever be in one place, so can write (* cough *, hack) something use-case-specific.

EDIT: initial proposal here: #64

@davestewart
Copy link
Owner

davestewart commented Apr 24, 2023

Hey @Nike682631 / @IlCallo ...

I'm just starting work on this and realised that Alias HQ does support comments, as it uses TypeScript's own parser to load the config.

We even have tests for it:

Are you perhaps using an older version?

FWIW I'm going to refactor anyway, as #41 requires it. I may use JSON5 or I may use get-tsconfig.

Additionally, this change will probably impact how Alias HQ resolves compilerOptions.paths as I think the current implementation might be wrong:

@Nike682631
Copy link
Author

@davestewart I'm pretty sure we were using the latest package version. I'll have to check this out again but I think adding aliashq was when the comments started to break the project.

@davestewart
Copy link
Owner

If you have time to check, that would be great. I'll continue work on this anyway

@davestewart
Copy link
Owner

Hey @Nike682631,

I just checked your PR:

https://github.com/quasarframework/quasar/pull/15702/files#diff-be6b822ab67a48c0e358715e97aa5d060dd9aa2cea5e44ab9ae59a9e28f680ddR58

Looks like that is v2.x and Alias is now on 6.1.0 and supported comments since 5.3.

So, update to the latest and you should be good to go!

I'll leave this open until you've had time to check.

@Nike682631
Copy link
Author

Hey @davestewart ,
Yep I rechecked. You're right. Feel free to close this. I'll reopen incase I have something else on this. Thanks for being so helpful and responsive.

@davestewart
Copy link
Owner

No problem. Thanks for using Alias HQ!

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

No branches or pull requests

3 participants