Skip to content
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

Merged
merged 7 commits into from Mar 3, 2017

Conversation

janvennemann
Copy link
Contributor

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.

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.
Copy link
Contributor

@hansemannn hansemannn left a 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));
Copy link
Contributor

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');
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@janvennemann
Copy link
Contributor Author

Addressed all comments and resolved conflicts with master branch.

@hansemannn
Copy link
Contributor

Please resolve the (latest) merge conflicts, should be the last one, thx!

@hansemannn
Copy link
Contributor

Approved!

@hansemannn hansemannn merged commit 4505b24 into tidev:master Mar 3, 2017
@janvennemann janvennemann deleted the TIMOB-23907 branch March 23, 2017 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants