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

Eliminate when.js and resolve several issues #315

Merged
merged 14 commits into from
Jan 10, 2021
Merged

Conversation

techfg
Copy link
Collaborator

@techfg techfg commented Jan 5, 2021

Resolves #311, Resolves #312, Resolves #313, Resolves #314, Resolves #316 & Resolves #317 - Plan on working on NPM support, updating jQuery, etc.

@techfg techfg changed the title Modernize Eliminate when.js and resolve several issues Jan 5, 2021
@jamietre jamietre self-requested a review January 5, 2021 12:05
@jamietre jamietre self-assigned this Jan 5, 2021
@jamietre
Copy link
Owner

jamietre commented Jan 5, 2021

Hi! I am thrilled that someone has taken an interest in this project. Obviously I've completely neglected it, largely because I haven't been actively in any of my work for a long time. But I think it's still useful; while SVG has universal support now and is probably better for some simple use cases, other use cases (particularly those involving altImage) do not have an elegant, simple solution in modern HTML as far as I know. I've thought many times about trying to bring this out of the dark ages but it's been hard to prioritize it since I don't personally use it right now.

First and foremost, it probably makes sense for me to just make you a collaborator, since it's clear you have much more investment right now in the project's future than I do. Let me know if you want me do that and you can self manage this.

At this point, since I've let this project die on the vine, I don't think I deserve a lot of say in where you go with this. But if you want I can be a reviewer and advisor as time permits. The things that I would probably do if I started from scratch are:

  1. rewrite it in TypeScript -- or at a minimum, publish types for the API to make it friendly to typescript projects. I might be able to help with this if you don't do TypeScript
  2. create a react component that wraps it which would make it much more accessible to many users as an alternative to the direct fluent API. I haven't thought about what this would look like but probably just expose the API events/actions as props; the imagemap itself could be a child?
  3. get rid of when - you did this
  4. get rid of the jQuery/zepto dependency. Though it might be easier to pull it out of the dark ages by keeping that api internally, since that's how it was written, I doubt there's a lot that it needs that doesn't have a straightforward implementation that's compatible with all modern browsers nowadays. It looks like you reimplemented many of the methods anyway to make it compatible with the zepto subset, it probably wouldn't take much more to just reimplement the core methods that remain from zepto and not even need that.
  5. get rid of when which you did already
  6. get rid of iqtest (maybe you did that?) and use a well-used framework like jest, ava or even something older like mocha. At the time I wrote iqtest, promises were pretty new and poorly supported by existing test frameworks (qunit??) but it doesn't make any sense now. The tests are pretty simple iirc
  7. get rid of ruby dependency for the build, and browserify, there's nothing that can't be done much more directly with node/webpack/something more modern than webpack
  8. publish to npm as you've already planned
  9. update & migrate the website to gatsby or whatever and host it on github, then I can cancel my discountasp.net account where the documentation is hosted

Beyond that there's probably a lot of terrible code, I was a pretty inexperienced JS coder at the time ;)

What you choose to do here is totally up to you though assuming you want to basically take over here, those are just the things that make sense to me in the javascript world I live in these days.

@techfg
Copy link
Collaborator Author

techfg commented Jan 5, 2021

Hi @jamietre -

First off, thanks for all your work on this plugin over the years! Secondly, really appreciate your quick response to my PR and sharing your thoughts, completely agree with your points on ways to improve/modernize. For my situation, I likely won't need to take things as far as they could be taken but definitely some of the items you mention I plan on tackling in the short term. I just inherited a project that used IM and moving to SVG isn't an option for now so modernizing the library is the best near term path forward.

For this PR, the number of files changed is a bit misleading, the actual code changes I made thus far are very minimal:

#311 - No code changes, just clean-up by running build from project root to normalize whitespace and also build latest dist folder since the last commit to the library didn't include a new "build"
#312 - Not clear why github is showing the entire file in the diff but only 2 lines of code changed
#313 - Only 2 lines changed
#314 - Only core.js for utils.when & utils.defer changed
#316 - A few lines of code changed related to locating altimages

Regarding your input/ideas, a couple of thoughts/questions:

4. get rid of the jQuery/zepto dependency - Just FYI that I didn't touch anything on the zepto side across the 4 commits, only changes to that file are normalizing whitespace. I'm not familiar with Zepto so if there's anything I would need to do/fix here as I modernize, please let me know your thoughts. Alternatively, possibly it makes sense to remove the "Zepto" specific elements all together?
7. get rid of ruby dependency for the build - Was thinking webpack here but I don't play in that space typically so do you think another build framework would be more appropriate (you mention "something more modern than webpack")?

Regarding next steps:

  1. Happy to be made a contributor to lessen the burden on you.
  2. I saw that you assigned yourself as a reviewer for the PR. If you do have the time and could review, would be great to get your eyes on these initial changes to make sure I didn't do anything that would comprise existing functionality/usability.

Thanks again!!

@jamietre
Copy link
Owner

jamietre commented Jan 6, 2021

I made you a collaborator.

Thanks for clarifying the changes! jquery/zepto - obviously I completely forgot that I did that at some point ;)

I will be glad to take a look at the code, though chances are my opinion will have very little to do with making sure nothing changes on the overall project operation since I doubt I remember much at all about it! But probably better than nothing!

@techfg
Copy link
Collaborator Author

techfg commented Jan 6, 2021

@jamietre -

Thanks for making me a collaborator, I just accepted invite, happy to join the team!

I made a few more changes last night, all minimal but a few fixes and updates to docs, etc. The changelog outlines everything that's included/target for the next release (1.2.14), what's planned for 2.0 and based on your input, what a roadmap could entail.

When you get a chance:

  1. Let me know your thoughts on the changes. I'll likely move forward with a release but would appreciate your review if you have the cycles just to make sure everything seems in line, at least from what you can remember :)
  2. Do you think it makes sense to eliminate Zepto support completely or since it appears to work as is, leave it until the time comes when a more thorough re-write/refactor is completed?
  3. In your previous comments you mentioned possibly using something more "modern than webpack" - any insight/thoughts on what build/packaging system you think should be used?

Thanks again!

@techfg techfg merged commit a971464 into jamietre:master Jan 10, 2021
@techfg techfg deleted the modernize branch January 10, 2021 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment