Allow node core deps to be used and whitelisted #479
Conversation
matteofigus
changed the title
| @@ -58,6 +58,7 @@ | |||
| "babili-webpack-plugin": "0.0.11", | |||
| "basic-auth-connect": "^1.0.0", | |||
| "body-parser": "^1.16.0", | |||
| "builtin-modules": "^1.1.1", | |||
There was a problem hiding this comment.
a little convenience module with a list of all the core node dependencies
| @@ -17,7 +19,7 @@ module.exports = function(components){ | |||
| components.forEach((c) => { | |||
| const pkg = fs.readJsonSync(path.join(c, 'package.json')); | |||
| const type = pkg.oc.files.template.type; | |||
| const dependencies = pkg.dependencies; | |||
| const dependencies = pkg.dependencies || {}; | |||
There was a problem hiding this comment.
this comes from package.json. After testing, I found a component with no dependencies props could actually generate some troubles
| @@ -43,7 +43,7 @@ module.exports = function(components){ | |||
| }); | |||
|
|
|||
| return { | |||
| modules: _.keys(deps.modules), | |||
| modules: _.union(coreModules, _.keys(deps.modules)).sort(), | |||
There was a problem hiding this comment.
This is used for the CLI. Now the cli by default allows any core module to be used. When running in the registry, still, the dependency will need to be whitelisted as all the other deps in order to be usable.
There was a problem hiding this comment.
Why are we sorting them?
There was a problem hiding this comment.
Good point. I think it was to make a test green but I can sort the result in the test instead as this is not necessary here.
| return !_.includes(_.keys(dependencies), dep); | ||
| }; | ||
| const missingExternalDependency = (dep, dependencies) => | ||
| !_.includes(_.keys(dependencies), dep) && !_.includes(coreModules, dep); |
There was a problem hiding this comment.
This part is used for the CLI bundler. This is for making the bundler bypass the dependency in case is a core, by being treated in the same way normal deps are.
| @@ -3,7 +3,7 @@ | |||
|
|
|||
| const webpack = require('webpack'); | |||
| const path = require('path'); | |||
| const externalDependenciesHandlers = require('./externalDependenciesHandlers'); | |||
| const externalDependenciesHandlers = require('./external-dependencies-handlers'); | |||
There was a problem hiding this comment.
just a little renaming for consistency
| code: strings.errors.registry.DEPENDENCY_NOT_FOUND_CODE, | ||
| missing: [modulePath] | ||
| }; | ||
| throw getError(requirePath); | ||
| } |
There was a problem hiding this comment.
So the flow here is 1) try to require the module normally 2) if it fails and is a core dep, try to require it 3) if it fails, return the error at the end
| @@ -43,7 +43,7 @@ module.exports = function(components){ | |||
| }); | |||
|
|
|||
| return { | |||
| modules: _.keys(deps.modules), | |||
| modules: _.union(coreModules, _.keys(deps.modules)).sort(), | |||
There was a problem hiding this comment.
Why are we sorting them?
| const requireDependency = (requirePath) => { | ||
| const nodeModulesPath = path.resolve('.', 'node_modules'); | ||
| const modulePath = path.resolve(nodeModulesPath, requirePath); | ||
| return require(modulePath); |
There was a problem hiding this comment.
I'll perhaps move the try catch logic inside each requiring function requireCoreDependency and requireCoreDependency .
There was a problem hiding this comment.
Ok done. I found a try-require module for that and made some more cleanup around.
There was a problem hiding this comment.
Nice we should then use it also for the templates as we are doing similar stuff in multiple places, perhaps once we work on allowing multiple-version deps
| code: strings.errors.registry.DEPENDENCY_NOT_FOUND_CODE, | ||
| missing: [modulePath] | ||
| }; | ||
| throw getError(requirePath); |
There was a problem hiding this comment.
getError accept a param called moduleName and here you are passing a path, perhaps worth renaming such param to accomodate both cases better?
| code: strings.errors.registry.DEPENDENCY_NOT_FOUND_CODE, | ||
| missing: [modulePath] | ||
| }; | ||
| throw getError(requirePath); | ||
| } |
| throw getError(requirePath); | ||
| } | ||
| } | ||
| return requireDependency(requirePath) || requireCoreDependency(requirePath) || throwError(requirePath); |
Fixes #462