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

Modernize Distribution #8

Closed
curran opened this issue Dec 29, 2020 · 6 comments
Closed

Modernize Distribution #8

curran opened this issue Dec 29, 2020 · 6 comments

Comments

@curran
Copy link
Contributor

curran commented Dec 29, 2020

From a discussion in #1

User story: As a potential user of this library, I would like to see it have a modern and easy to consume distribution, so that I can use it in my project (either via NPM as a dependency, or via some CDN such as JSDelivr or UNPKG).

I suppose the ideal scenario would be a UMD bundle distribution that contains dependencies as well (science, tiny-queue).

@curran
Copy link
Contributor Author

curran commented Dec 29, 2020

Suggested approach: replace Makefile with Rollup.

Reference example: https://github.com/rollup/rollup-starter-lib

@curran
Copy link
Contributor Author

curran commented Dec 29, 2020

As I dig in a little bit, I am able to run make install and make test.

I notice that the tests are not all passing. Here is the output:

> reorder.js@1.0.7 test /home/curran/repos/reorder.js
> vows --nocolor; echo

·· ·· · ·· ·· ··· ··· · · ··· ·· ··· ·· ·····✗✗✗✗✗✗✗✗✗✗ ·· · ··· ·······  
   
   
    order 
      ✗ insert-simple 
      Invalid list, indices not sorted 
   
      ✗ insert-1 
      Invalid list, indices not sorted 
   
      ✗ insert-2 
      Invalid list, indices not sorted 
   
      ✗ insert-3 
      Invalid list, indices not sorted 
   
      ✗ insert-4 
      Invalid list, indices not sorted 
   
      ✗ insert-5 
      Invalid list, indices not sorted 
   
      ✗ insert-6 
      Invalid list, indices not sorted 
   
      ✗ insert-7 
      Invalid list, indices not sorted 
   
      ✗ insert-lesssimple 
      Invalid list, indices not sorted 
   
      ✗ insert-evenharder 
      Invalid list, indices not sorted 
  ✗ Errored » 45 honored ∙ 10 errored (0.604s) 

Is this expected? Or were the tests all passing in the past?

My gut feeling is that the first task should actually be to make sure all the tests pass, then only after that we can go about working on the distribution.

Also, a few notes from initial investigation:

The Makefile concatenates the source files together. Options for moving forward with Rollup include:

  • Modifying all source files to use ES6 modules (import and export). This would be a lot of work.
  • Setting up Rollup to consume as input the concatenated output from the Makefile. This may be less work, and may be the shortest path to having a modernized distribution.

Another adjacent thought - if we were to really deeply refresh this library, we could also take a stylistic pass over the codebase and adopt let and const, and arrow function expressions. Also Prettier for code formatting would probably be a good move.

However, with the tests in a broken state, it does not seem feasible to move forward at all with any of these steps. We need a stable test harness to check what things break and how they break along the way towards modernization.

Anyone here familiar enough with the failing test cases to maybe taks a stab at making them pass (or removing them if they are not valid tests)?

@curran curran mentioned this issue Dec 29, 2020
@curran
Copy link
Contributor Author

curran commented Dec 29, 2020

Opened a separate issue for only fixing the tests: #9

@jdfekete
Copy link
Owner

I have merged the pull requests The new version compiles fine and passes the tests.
I will update the README file and acknowledge your work, @curran!

Now, how should the html examples in the examples directory be changed to work again?

@curran
Copy link
Contributor Author

curran commented Dec 30, 2020

Ah! The html examples in the examples directory can be made to work again by having them reference the UMD output in dist.

@curran
Copy link
Contributor Author

curran commented Dec 30, 2020

Closing this issue as it's essentially done.

Opening a few more as follow-on tasks:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants