davestewart / alias-hq

The end-to-end solution for configuring, refactoring, maintaining and using path aliases
https://davestewart.co.uk/projects/open-source/alias-hq/
MIT License
333 stars 12 forks source link

Change JSON parser to JSON5 #63

Closed Nike682631 closed 1 year ago

Nike682631 commented 1 year ago

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 commented 1 year ago

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

https://github.com/davestewart/alias-hq/blob/cde1ee04ed1e36a90b6909a6b8da5805923a990b/cli/utils/file.js#L12

to use JSON5.parse(text)

davestewart commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/davestewart/alias-hq/issues/64

davestewart commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

davestewart commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

No problem. Thanks for using Alias HQ!