-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
[TIMOB-23907] Fix metadata parsing for forward declarations #125
Conversation
A framework’s umbrella header needs to be included to properly resolve forward declaration in the metabase parser. This also includes a fix to avoid forward declarations being parsed as definitions which would result in empty definitions in the metabase.
This prevents the unnecessary generation of Hyperloop JS source files that are not used anywhere.
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.
Left a few logging-comments, no biggies. Can merge after being addressed.
usedClasses[className] = this.sourceSet.classes[className]; | ||
} else { | ||
var fqcn = classInfo.framework + '/' + classInfo.class.name; | ||
util.logger.trace(chalk.gray('exlcuding class ') + chalk.green(fqcn)); |
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.
Typo in exlcuding
, also capitalize.
* @param {String} outputPath Path where to save source code files to | ||
*/ | ||
generateClasses(outputPath) { | ||
util.logger.trace('Generating Hyperloop JS proxies for native classes'); |
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.
We're trying to avoid the term "proxies". Use "wrappers" instead.
@@ -172,7 +180,7 @@ function generateFromJSON (name, dir, json, state, callback, includes) { | |||
cls.framework = custom_frameworks[cls.filename] || cls.framework; | |||
} | |||
// TODO: add categories |
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.
Is this todo resolved by you rewriting the code-gen? Do we have a ticket for this otherwise?
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.
Actually, it could be that it's fixed because we now alway import the framework's umbrella header which should include categories. I found https://jira.appcelerator.org/browse/TIMOB-24186 for this issue. Let me test and get back to you.
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.
Did a test and it is indeed resolved through the use of the umbrella header. Methods from categories are now included in the metabase provided that they were imported in the frameworks umbrella header. However there is another issue with that ticket, but i will address that there.
Addressed all comments and resolved conflicts with master branch. |
Please resolve the (latest) merge conflicts, should be the last one, thx! |
Approved! |
JIRA: https://jira.appcelerator.org/browse/TIMOB-23907
Properly resolves forward declarations by always including a framework's umbrella header. To keep the number of generated files small, the code generation was changed. Unused classes will now be filtered out before actually generating all the Hyperloop JS files.