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

Define nodes as Array #21

Closed
wants to merge 1 commit into from
Closed

Conversation

AnSavvides
Copy link

This is a very simple change that does not alter the functionality in any way - it simply avoids the need to do a split on a String, as is the case at present, while also laying out all nodes supported in a more readable form.

By the way, awesome project - kudos for putting this together. Shame about the performance issues you identified, but I am sure at some point in time it should be possible to overcome this limitation.

@adriancooney
Copy link
Owner

Thanks for the PR! I understand where you're coming from but the reason I chose the split method vs an array was to save an extra 240 characters. As you can see here, having the tags within an array is 50% faster but at such large numbers and the fact that it's only done once at runtime, the performance impact on the browser would be neglible. In essence, the time spent loading the extra 240 characters could be spent splitting the array!

@AnSavvides
Copy link
Author

No worries, that's fair enough - I didn't think of it like that.

I believe the benefits (readability & slight speed-up) outweigh any drawbacks (240 extra characters) to do with these changes, but it's your call!

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.

2 participants