fix: update CLI to handle UTF8 BOM #1357
Conversation
ghost
assigned 
There was a problem hiding this comment.
There are still 9 places where JSON.parse(fs.readFileSync(.)) is still directly used, for example, amplify-category-analytics/commands/analytics/add.js Line 5.
All of them are for reading "servicesMetadata".
For consistency, that line can be moved inside the run function, and use the new approach to read.
| @@ -49,7 +49,7 @@ module.exports = { | |||
| */ | |||
| const currEnv = amplify.getEnvInfo().envName; | |||
| const teamProviderInfoFilePath = amplify.pathManager.getProviderInfoFilePath(); | |||
| const teamProviderInfo = JSON.parse(fs.readFileSync(teamProviderInfoFilePath)); | |||
| const teamProviderInfo = context.amplify.readJsonFile(teamProviderInfoFilePath); | |||
There was a problem hiding this comment.
For consistency, use amplify directly without the context.
There was a problem hiding this comment.
Lets do that later. If there are any problem in the ServiceMetaData the CLI will fail to start as they are at the top level
There was a problem hiding this comment.
I see your point, but don't you think it's better that this is consistent across the whole project? currently, some are on top of the file direct using JSON.parse(fs.readFileSync(.)) directly, such as amplify-category-analytics/commands/analytics/add.js; and some are inside the run method using context.amplify.readJsonFile(.), such as amplify-category-auth/commands/auth/enable.js
There was a problem hiding this comment.
I have updated the code to not to use context.amplify where amplify is available.
| const resourceDirPath = path.join(projectPath, 'amplify', 'backend', category, resourceName); | ||
|
|
||
| // Change CFN file | ||
|
|
||
| const cfnFilePath = path.join(resourceDirPath, 's3-cloudformation-template.json'); | ||
| const oldCfn = JSON.parse(fs.readFileSync(cfnFilePath, 'utf8')); | ||
| const oldCfn = context.amplify.readJsonFile(cfnFilePath, 'utf8'); |
There was a problem hiding this comment.
The deleted comment is here, you don't need to use encoding if the encoding is 'utf8'.
| @@ -349,7 +349,7 @@ function migrate(projectPath, resourceName) { | |||
|
|
|||
| // Change Parameters file | |||
| const parametersFilePath = path.join(resourceDirPath, parametersFileName); | |||
| const oldParameters = JSON.parse(fs.readFileSync(parametersFilePath, 'utf8')); | |||
| const oldParameters = context.amplify.readJsonFile(parametersFilePath, 'utf8'); | |||
There was a problem hiding this comment.
you don't need to use encoding if the encoding is 'utf8'.
| @@ -49,7 +49,7 @@ module.exports = { | |||
| */ | |||
| const currEnv = amplify.getEnvInfo().envName; | |||
| const teamProviderInfoFilePath = amplify.pathManager.getProviderInfoFilePath(); | |||
| const teamProviderInfo = JSON.parse(fs.readFileSync(teamProviderInfoFilePath)); | |||
| const teamProviderInfo = context.amplify.readJsonFile(teamProviderInfoFilePath); | |||
There was a problem hiding this comment.
I see your point, but don't you think it's better that this is consistent across the whole project? currently, some are on top of the file direct using JSON.parse(fs.readFileSync(.)) directly, such as amplify-category-analytics/commands/analytics/add.js; and some are inside the run method using context.amplify.readJsonFile(.), such as amplify-category-auth/commands/auth/enable.js
|
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Some editors and platforms can add UTF BOM at the begining of the file. Node JSON.parse doesn't
handle the BOM enchoding very well and chokes the CLI. Updating the CLI to use a util method which
can handle the BOM chars
fix #1355 and fix #1122
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.