-
Notifications
You must be signed in to change notification settings - Fork 21
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
[ecmascript] (#311) Fix for Unknown error on VROES.import from a missing/invalid module path #345
Conversation
…est indentation Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
Added more specific errors and default error handler (System.error). Removed unused Module.require, Module.export parameter, minor fixes Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
Renamed DEFAULT_ERR_HANDLER, minor formatting fix Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
…ues in README Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
} | ||
moduleInfo = !path ? null : System.getModule(path); | ||
if (!moduleInfo) { | ||
throw new Error(`No action or module found for paths: '${path}', '${path}/index'!`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could include moduleName, actionName as additional information to error message. In case of typos, strange chars, etc it could be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path is included for that purpose. It is unknown whether an action or a module (namespace) is being imported.
The specifiers (elements to be imported from a module, or default/*) are validated separately
packages/ecmascript/src/Module.ts
Outdated
actionResult = invokeActionOrModule(path, onError); | ||
} | ||
catch (err) { | ||
error = err?.message || err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best approach is always to store whole Exception object, instead of error message property only.
Small chance at TS/JS but whole exception/error object may contains some additional information such as stackTrace or internal error details.
Because error variable has initial value string then (something like this):
.... catch(err){ error = err.toString(); }...
…th Error objects Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
You need to increase your test coverage for this. You've added functionality that was not tested. |
Passing the error handler for each function sounds like a bad idea. We won't really use Notes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more tests and move error handling away from each method
Merged latest changes from main (as a "chore" squashed commit)
|
@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
1 similar comment
@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
89b654c
to
16c50af
Compare
Tested successfully on an environment, small fix was required. |
…ng options, regex validation New enum with error handling options for the Module.load and import methods. New method setModuleErrorHandler in VROES and Module, accepting enum option or custom error handler Regex validation of action/module paths in Module.load/import Module error handling will only be invoked at the public method (load/import) level (not recursively) Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
Enum export was causing issues with generated JS Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
16c50af
to
4ba47d7
Compare
@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
1 similar comment
@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
…ort-unknown-error Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
4ba47d7
to
680b867
Compare
Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
Signed-off-by: bcpmihail <pmihail@vmware.com>
Conflicts resolved. @Michaelpalacce @dimitar-nikolov-lazarov please review. |
@dimitar-nikolov-lazarov @Michaelpalacce whenever you have the chance to check |
…ix/issue-311-vroes-import-unknown-error Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
packages/ecmascript/src/Module.ts
Outdated
* - one of the predefined {@link DefaultModuleErrorHandlers} options | ||
* Default is {@link DefaultModuleErrorHandlers.SYS_ERROR} | ||
*/ | ||
setModuleErrorHandler(eh: DefaultModuleErrorHandlers | ModuleErrorHandler): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to be accepting a string
or a function
? It's breaking the "S" in SOLID.
packages/ecmascript/src/Module.ts
Outdated
/** | ||
* Predefined handlers for errors on loading/importing a module or action - see {@link DefaultModuleErrorHandlers} | ||
*/ | ||
const ModuleErrorHandlers = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't think this will ever change.. we give them a default and they can overwrite it if they want, imho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Keeping only the System.error option as default (will also address #345 (comment) )
* Note - use of '-' and capital letters is not limited, despite not being recommended. | ||
* Using non-capturing group (?:) to reduce overhead. | ||
*/ | ||
const IMPORT_BASE_REGEX = /^(?:[\w-]+\.)*[\w-]+$/g; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets give examples for these regexes?
packages/ecmascript/src/Module.ts
Outdated
@@ -94,6 +205,7 @@ export interface ModuleElementList { | |||
return this; | |||
}; | |||
|
|||
/** @deprecated - unused and not part of {@link ModuleExport} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecations should happen after an internal triage, lets add this to the roadmap to discuss?
* @throws Error if the relative path goes too many steps back (via ../) in the base path | ||
*/ | ||
|
||
function constructAbsolutePath(path: string, base: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be extensively unit tested. What I mean here: ONLY this method needs to be unit tested. With a data provider that will have: path, base, expectedFailure, expected or sth like that
* @throws Error if there are no specifiers | ||
* @throws Errir containing a list of invalid specifiers (blank, duplicate, missing from the provided module), if any. | ||
*/ | ||
function extractImports(specifiers: string[], importedModule: any): any[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous comment about unit testing
…ault module error handler Addressing PR comments - removed enum with module error handler options Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
…emoved @deprecated Pending discussion to re-add @deprecated for Module.Export.from` Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
60aec81
to
5687f7c
Compare
LGTM |
…odule.constructAbsolutePath Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
…odule.extractImports Signed-off-by: Mihail Penchev (c) <pmihail@vmware.com>
Description
Issue summary:
VROES.import(...).from(invalidModulePath) threw an Unknown error. Cause was calling System.getModule() with blank path.
Changes:
Added validation for module path and specifiers in ecmascript/Module, affecting for VROES.import().from().
Additional parameter for handling the validation errors in Module.import(), Module.load() with default value - function to log the error and return null (avoids breaking changes).
Removed unused path parameter and from() method from export.
Documentation (comments, README, release note)
Unit tests.
Checklist
Fixed #XXX -
orClosed #XXX -
prefix to auto-close the issueTesting
Tested on a client environment with the same action as the issue and a library that searched through potential module paths.