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

changed dustpress.js to class format and added eslint configs #23

Open
wants to merge 5 commits into
base: webpack
Choose a base branch
from

Conversation

godbone
Copy link
Contributor

@godbone godbone commented Jan 25, 2019

No description provided.

js/dustpress.js Outdated

this.instance.successHandler = ( data, textStatus, jqXHR ) => this.successHandler( data, textStatus, jqXHR );
this.instance.errorHandler = ( jqXHR, textStatus, errorThrown ) => this.errorHandler( jqXHR, textStatus, errorThrown );
this.instance.uploadProgressHandler = ( event ) => this.uploadDownloadProgressHandler( event );

Choose a reason for hiding this comment

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

I am not sure if this will cause events to collide if two requests are performed simultaneously. The issue here might be that the methods are now a part of the DustPress class, not the instance created request instance itself.

Have you tested this and if so, could you clarify how this changed format ensures instance uniqueness compared to the old implementation?

Copy link

@villesiltala villesiltala left a comment

Choose a reason for hiding this comment

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

Great updates! 👍 Take a look at a couple of questions in the request class.


this.params.start();

this.params.successHandler = ( data, textStatus, jqXHR ) => this.successHandler( data, textStatus, jqXHR );

Choose a reason for hiding this comment

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

What is the purpose of recreating these methods under the "params" key? I can't see the need for this since the methods are already accessible under the same scope. If you're thinking the class methods are not confined to a single object, I believe it is not the case. Each request instance should have its own methods which can be referenced safely for instance by assigning them as event handlers.

this.params.downloadProgressHandler = ( event ) => this.uploadDownloadProgressHandler( event );

return $.ajax( options )
.done( ( data, textStatus, jqXHR ) => this.params.successHandler( data, textStatus, jqXHR ) )

Choose a reason for hiding this comment

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

Referring to my comment above, why can't this just be:

.done( ( data, textStatus, jqXHR ) => this.successHandler( data, textStatus, jqXHR ) )

this.successHandler here refers to a method of an instance since it is not static. Thus it should be the same thing as using this.params.successHandler.

* @param {object} parsed Parsed data.
*/
addToDebugger( parsed ) {
delete this.params.params.success;

Choose a reason for hiding this comment

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

Where could these come from and why are they removed here?

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