Refactor archinfo.js to typescript #10624
Conversation
pmogollons
commented
Jul 11, 2019
•
pmogollons
commented
- Updated code to use modern JS
- Added types
- Stopped using 2 underscore functions (1 remaining)
- TS complains about execFileSync not being exported. I think it requires the update of that file too.
tools/utils/archinfo.ts
Outdated
| @@ -1,7 +1,7 @@ | |||
| var _ = require('underscore'); | |||
| var os = require('os'); | |||
| const { max } = require('underscore'); | |||
There was a problem hiding this comment.
As I understand, the reason this wasn't transformed into an import statement is because there wasn't a type definition for underscore. A better way to solve this would be to install @types/underscore. The local tools/node_modules is a symlink to dev_bundle/lib/node_modules so I'm currently trying to find which package.json should be changed in order to add the package.
Since dev_bundle changes can be made just by the MDG maybe @benjamn or someone else could help with this. I'm currently trying to figure out which particular package.json has to be changed.
There was a problem hiding this comment.
Sure, but the main reason for not going that way was taking a look to a file that @benjamn already refactor to TS and was requiring underscore instead of importing it and installing the types.
But installing the types should be the way to go. Or even better just removing underscore completely. But that requires some additional work.
There was a problem hiding this comment.
Good catch, I'm still new to the codebase so it's hard to understand the reasoning behind leaving the require.
There was a problem hiding this comment.
Looks good @pmogollons! I changed the import from tools/utils/utils.js back to use require for now, because of the execFileSync issue. That should be resolved when #10634 is merged.
* Updated code to use modern JS * Added types * Stopped using 2 underscore functions (1 remaining)
|
Awesome. Thanks. |
