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

Cleanup spree.js (and rewrite as plain JS) #1754

Merged
merged 2 commits into from
Mar 7, 2017

Conversation

jhawthorn
Copy link
Contributor

Previously we were declaring the root window.Spree object as a CoffeeScript class. This was harmless, because it was compiled into an empty function, but also unnecessary, because the object it created had no purpose. This replaces the window.Spree object with a normal object, which should be somewhat more inspectable when viewed in a devtools console.

Before:

After:

This also rewrites the file as vanilla JS. Normally we avoid converting our files between coffee and JS, but this is the only JS file in the core project, so I think it is worth replacing. The entire file was already changed in the previous commit, so I believe this is a good time to make the change.

Previously we were declaring Spree to be a CoffeeScript class. This was
harmless, because it was compiled into an empty function, but also
unnecessary, because it had no class methods and was never used as a
class.
This file's history was already replaced by the previous commit, so we
might as well convert to plain JS at the same time.

This is the only CoffeeScript (now only JS) file in the solidus_core
gem, so it can now in theory be used in an app without needing to pull
in CoffeeScript (assuming the app doesn't include solidus_frontend or
soldius_backend).
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks :shipit:

@jhawthorn jhawthorn merged commit 80d63c2 into solidusio:master Mar 7, 2017
@jhawthorn jhawthorn deleted the cleanup_spree_js branch March 7, 2017 19:39
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