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

loading json contributions files fails if the file has a BOM at the start #1039

Open
hahn-kev opened this issue Aug 5, 2024 · 3 comments
Open
Labels
bug Something isn't working

Comments

@hahn-kev
Copy link

hahn-kev commented Aug 5, 2024

From what I can tell the offending code is here:

try {
// TODO: Provide a way to make sure extensions don't tell us to read files outside their dir
const localizedStringsJson = await nodeFS.readFileText(
joinUriPaths(extension.dirUri, extension.localizedStrings),
);
const localizedStringsDocument = JSON.parse(localizedStringsJson);
localizedStringsDocumentCombiner.addOrUpdateContribution(
extension.name,
localizedStringsDocument,
);
} catch (error) {
logger.warn(
`Could not add localized strings contribution for ${extension.name}: ${error}`,
);
}
}
if (extension.menus) {
try {
// TODO: Provide a way to make sure extensions don't tell us to read files outside their dir
const menuJson = await nodeFS.readFileText(
joinUriPaths(extension.dirUri, extension.menus),
);
const menuDocument = JSON.parse(menuJson);
menuDocumentCombiner.addOrUpdateContribution(extension.name, menuDocument);
} catch (error) {
logger.warn(`Could not add menu contribution for ${extension.name}: ${error}`);
}
}

The error that I got was this:

[start:core] 15:32:48 
[start:core] [extension host warning]
[start:core] 15:32:48.998 > Could not add menu contribution for fw-lite-extension: SyntaxError: Unexpected token '', "{
[start:core]   "mai"... is not valid JSON [at C:\dev\paranext-core\src\extension-host\services\extension.service.ts:1033:18]
[start:core] [/extension host warning]

at the very least it would be nice if there was better feedback about what the cause was (after an error check if the json starts with a BOM), and indicate that. Best case, the build step should strip the BOM, or the code could remove it at runtime.

@tjcouch-sil tjcouch-sil added the bug Something isn't working label Aug 5, 2024
@lyonsil
Copy link
Member

lyonsil commented Aug 9, 2024

This appears to be a minor controversy with node fs libraries.

It should be easy enough to fix in our fs library wrapper by adding .replace(/^\uFEFF/, '') to the returned value. Or we can install the 58M weekly downloads package with a few lines of code that does this.

@hahn-kev
Copy link
Author

Wow that's a lot of related issues.

I'd be happy to make a PR, are you thinking it would be done at runtime? That would be the simplest solution.

@lyonsil
Copy link
Member

lyonsil commented Aug 12, 2024

@hahn-kev If you're happy to make a PR go for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: 📥 On Deck
Development

No branches or pull requests

3 participants