Add NPM package - Githubissues

TheSpyder commented 7 months ago

Fixes #3

The basics should make sense but there are some possibly unexpected changes:

I called it @dmsnell/diff-match-patch because https://github.com/JackuB/diff-match-patch took the top-level diff-match-patch name on NPM. Feel free to rename it.
I deleted the minified code, because it's not really necessary in an NPM package. Any project that needs it will already have tools to do this.
I added zora to run the tests, it's lightweight and fast

I'd like to also add the types from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/diff-match-patch but they're technically MIT. The types are very simple, and there's probably no legal issue with copying them, but I don't know if you want to deal with potentially mixed licensing 🤔

TheSpyder commented 7 months ago

I wasn't sure what to do with the readme in the NPM package, so I left it out. https://github.com/JackuB/diff-match-patch ships what I think is a copy of the wiki API from google.

dmsnell commented 7 months ago

@TheSpyder I'll be on vacation during the next week, but I'll try and get to this once I'm back. some initial thoughts:

are we sure this export change leaves existing code and dependencies in a good state?
I wonder if we can stage this in multiple pieces. one thing I thought would be nice is to start building the minified and compressed version of the library in Github Actions. it'll be nice to leave things in as they are for existing projects, but if we can remove the need to manually run build steps that would be awesome.
despite being slower and possibly silly, I'd probably prefer leaving major changes like revamping the tests or removing the compressed/minified version of this out of a PR to start publishing an npm package. I get quickly overwhelmed and find it hard to focus on changes that do many things at once.

this is impressive I can tell even at a glance. great work.

TheSpyder commented 7 months ago

are we sure this export change leaves existing code and dependencies in a good state?

Hard to say. For anyone using the NPM package, it's equivalent, but I did see one person in the issues of that repo asking for a simple change to keep it running in browsers where a module global isn't available. I'll add that.

I wonder if we can stage this in multiple pieces. one thing I thought would be nice is to start building the minified and compressed version of the library in Github Actions. it'll be nice to leave things in as they are for existing projects, but if we can remove the need to manually run build steps that would be awesome.

That's definitely a good thought. I wanted to get something published for an internal deadline, but I might link the project to a branch of my fork rather than try to rush this.

despite being slower and possibly silly, I'd probably prefer leaving major changes like revamping the tests or removing the compressed/minified version of this out of a PR to start publishing an npm package. I get quickly overwhelmed and find it hard to focus on changes that do many things at once.

Fair point. I'll roll back the branch to remove the test change commit and wait until the NPM package settles to submit it.

I removed the minified version mostly I figured both should be equivalent and I don't have the tools installed to generate it from the uncompressed changes I made. I can bring it back and set the NPM package to publish without it.

this is impressive I can tell even at a glance. great work.

thank you 😁

TheSpyder commented 7 months ago

@dmsnell I've done a few things:

rolled back the test change (and kept it + added types on a branch we'll use for our release).
made some changes to attempt to keep the browser compatibility intact.
restored the compressed version, although it doesn't have the patches applied so the two files have diverged for the moment.

TheSpyder commented 6 months ago

@dmsnell will you have time to look at deploying this to NPM soon? I'm not trying to pressure you, just curious.

dmsnell commented 6 months ago

will you have time to look at deploying this to NPM soon? I'm not trying to pressure you, just curious.

sorry @TheSpyder - I should have anticipated this, but I'm currently held up by some work in the current WordPress release, which is happening right now. please allot me another week and I'll try to prioritize it starting on Monday, if not over the weekend.

TheSpyder commented 6 months ago

Monday is fine - I think by Wednesday we might have to look at an internal deployment. We're currently using a git link to a branch of my repo (from before I rolled back the tests).

TheSpyder commented 6 months ago

The code changes are all coming from https://github.com/JackuB/diff-match-patch which was adjusted to better suit use embedded in a JavaScript application, as NPM developers expect, where the Google code is based on globals and standalone use. I don't have much knowledge about them other than this setup works in our application.

The Diff api change, for example, was actually a rollback - a google patch in 2018 made new Diff tuple objects, but they're non-iterable which breaks ES6 environments. So it was swapped back to a plain array. https://github.com/JackuB/diff-match-patch/issues/14

start by automating the build process of the existing repo

I admit I have no idea what that is 😂 I have only looked seriously at the javascript folder.

dmsnell commented 6 months ago

this all reminds me how important it feels to get a CI job established for building the JS module. if you want to speed things up, you can work on that before I get to it. that is, start by automating the build process of the existing repo, then once that's in place, modify it so that it builds the npm package automatically.

or we could do this in one step, but only when ensuring that nothing breaks backwards compatbility.

on second thought this change of repo is a great opportunity to break from all the constraints from diff-match-patch, like Closure compiler. I forget that nobody has this official repo in their npm projects because it's never published one.

let's see if we can get a CI job setup which produces a minified build, and let's compare that output to what Closure Compiler has been producing. I'm inclined to try something like biomejs which doesn't pull in a bunch of dependencies.

TheSpyder commented 6 months ago

I forget that nobody has this official repo in their npm projects because it's never published one.

That's the key I'm focusing on - anyone using this library on NPM is doing npm install diff-match-patch and perhaps assuming it's the same, as my team did, but it's not. https://www.npmjs.com/package/diff-match-patch

dmsnell commented 4 months ago

@TheSpyder In #9 I eagerly added the Symbol.iterator method so hopefully that makes this PR a bit simpler. You are welcome to incorporate that change by merge or rebase or however you want.