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

Fixing codegen issues with reserved classes #945

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

inightingalesc
Copy link

@inightingalesc inightingalesc commented Nov 21, 2017

-Added built in classes String and Array to reserved words
-Sanitized all names in the namespace before building the namespace to avoid issue with constructors and interfaces

@inightingalesc inightingalesc changed the title Inightingalesc/fix codegen issues Fixing codegen issues Nov 21, 2017
@dcodeIO
Copy link
Member

dcodeIO commented Nov 24, 2017

The initial issue should have been fixed by merging #913. Not sure about the other changes because #870 (comment).

@inightingalesc
Copy link
Author

Thank you for taking a look at this, and for merging #913!

To give you some context for the other issue, the current implementation generates the following code when given a message with the name of a built-in javascript class (e.g, String):

SpecialString.String = (function() {

    /**
        * Properties of a String.
        * @memberof root.context.SpecialString
        * @interface IString
        * @property {string|null} [locale] String locale
        * @property {string|null} [text] String text
        */

    /**
        * Constructs a new String.
        * @memberof root.context.SpecialString
        * @classdesc Represents a String.
        * @implements IString
        * @constructor
        * @param {root.context.SpecialString.IString=} [properties] Properties to set
        */
    function String(properties) {
        if (properties)
            for (var keys = Object.keys(properties), i = 0; i < keys.length; ++i)
                if (properties[keys[i]] != null)
                    this[keys[i]] = properties[keys[i]];
    }

    /**
        * String locale.
        * @member {string} locale
        * @memberof root.context.SpecialString.String
        * @instance
        */
    String.prototype.locale = "";

This will fail at runtime because it will use the reserved String class instead of our new one.

Now, if I add Array|String to the list of reserved words, we have improvement, but the constructor now has the wrong name:

SpecialString.String_ = (function() {

                /**
                 * Properties of a String.
                 * @memberof root.context.SpecialString
                 * @interface IString
                 * @property {string|null} [locale] String locale
                 * @property {string|null} [text] String text
                 */

                /**
                 * Constructs a new String.
                 * @memberof root.context.SpecialString
                 * @classdesc Represents a String.
                 * @implements IString
                 * @constructor
                 * @param {root.context.SpecialString.IString_=} [properties] Properties to set
                 */
                function String(properties) {
                    if (properties)
                        for (var keys = Object.keys(properties), i = 0; i < keys.length; ++i)
                            if (properties[keys[i]] != null)
                                this[keys[i]] = properties[keys[i]];
                }

                /**
                 * String locale.
                 * @member {string} locale
                 * @memberof root.context.SpecialString.String_
                 * @instance
                 */
                String_.prototype.locale = "";

It seemed to me the simplest way to fix this is to escape the names of all types initially, so that the Type constructor generator doesn't need to worry about the function name.

Let me know what you suggest in terms of approach!

@inightingalesc inightingalesc changed the title Fixing codegen issues Fixing codegen issues with reserved Classes Nov 27, 2017
@inightingalesc inightingalesc changed the title Fixing codegen issues with reserved Classes Fixing codegen issues with reserved classes Nov 27, 2017
@mfedderly
Copy link

Not sure if this PR is still alive, but the same issue happens for Object as well as String and Array

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.

3 participants