Skip to content

Simplify/remove ixml.js #46

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

Closed
kadler opened this issue Mar 26, 2019 · 2 comments · Fixed by #116
Closed

Simplify/remove ixml.js #46

kadler opened this issue Mar 26, 2019 · 2 comments · Fixed by #116
Milestone

Comments

@kadler
Copy link
Member

kadler commented Mar 26, 2019

ixml.js includes various definitions for XMLSERVICE tags and attributes. This is way overly complicated and over-engineered.

For instance, I_XML_ATTR_KEY_ERROR is defined to the string "error". Having definitions like this can be useful, however:

  • this string will never change as it would break compatibility
  • "error" is in the key name, so you already have to know what you're looking for
  • I_XML_ATTR_KEY_ERROR is longer and more complicated to type (caps, underscores)

This abstraction brings no benefits whatsoever, while imposing extra mental effort to understand and parse the abstraction.

A better model would be to use Javascript template literals or possibly a template engine (though this might be overkill as well).

@markdirish
Copy link
Contributor

I think when we start removing these constants, and using the literals strings in the templates for the nodes, we will see that there are only a few dozen 'node templates' with a few options for each that need to be accounted for. Also the fact they they are all declared, and then all exported makes the file a lot larger than it will eventually end up.

I think native template literals are a great idea, and should probably be the starting point until we find a deficiency.

@abmusse
Copy link
Member

abmusse commented Aug 27, 2019

@kadler

After merging PR #80 can we close this issue?

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 a pull request may close this issue.

3 participants