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

Split up src/sweetalert2.js #970

Closed
zenflow opened this issue Mar 1, 2018 · 6 comments
Closed

Split up src/sweetalert2.js #970

zenflow opened this issue Mar 1, 2018 · 6 comments
Assignees
Labels
domain: devel Development-related, affecting contributors type: refactor

Comments

@zenflow
Copy link
Member

zenflow commented Mar 1, 2018

1403 lines of code is wayyyy too much for one module. We have a module bundler set up in our build pipe-line, so lets take advantage of it! All the constant scrolling is killing me when I work on this code. If we split up the code into multiple modules, we can (a) find things faster, and (b) easily work in more than one area at a time.

I don't really care how it's split up, so long as it's reasonable, and so long as it's split up.

Like I mentioned here #865 (comment) it's possible to move code into methods on a SwalContext object. I think we should do that, and also extract more utility functions.

Is anyone opposed to this? Either in general, or at this specific time? Obviously there will be a lot of code change to review and it could result in merge conflicts, but we can try to get all done quickly and at once.

@zenflow
Copy link
Member Author

zenflow commented Mar 1, 2018

/cc @limonte @toverux @ACupaJoe @samturrell @FinesseRus

@samturrell
Copy link
Contributor

Agreed, partly why #824 was taking so long. My new code is 95% contained in its own modules so won't cause me too much pain when it is ready to merge.

Module at a time might make it easier to review and reason about, if that's possible.

@zenflow
Copy link
Member Author

zenflow commented Mar 1, 2018

Module at a time might make it easier to review and reason about, if that's possible.

Good idea. Then I imagine it would be best to start with extracting the utility functions, and do the SwalContext.js module after that.

@zenflow
Copy link
Member Author

zenflow commented Mar 11, 2018

As soon as PR #1008 settles, put proper static methods (simply any and all methods that don't use the top-level currentContext variable) in src/static/*.js

@zenflow
Copy link
Member Author

zenflow commented Mar 27, 2018

So, update: the former main module has been completely dissolved. Now the entry point is 4 lines of code including blank lines, the module containing the Swal class/function is 72, and there are many small modules in src/utils, src/staticMethods and src/instanceMethods. Though instanceMethods/_main.js is still pretty big at 548 LOC and should be broken down. I'm pretty sure that is by far the largest module.

The issue was "Split up src/sweetalert2.js" so I think it's fair to say this can be closed

@limonte
Copy link
Member

limonte commented Mar 27, 2018

Great work @zenflow 💪 💪 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: devel Development-related, affecting contributors type: refactor
Projects
None yet
Development

No branches or pull requests

4 participants