Issues and v2 work updates

[karnthis]

Work to reduce outstanding issues new and old. Impacted items include:

• decompressFromBase64 can return null #184
• Version 2 TODO list #177
• Somehow compressToBase64() returns string end with 3 equal sign "===" #110
• Fixed safe uri alphabet (+$ -> _.) for compressToEncodedURIComponent/decompressToEncodedURIComponent #127

List to be updated as work is completed.

[Rycochet]

This isn't doing quite what you expect - previously it was running the build if it hadn't run already - we actually don't need that conditional run, so can just have vitest on its own - I was mixing up vite preview and vitest!

Suggested change

	"test": "[ -f ./dist/index.js ] || npm run test:build",
	"test:build": "npm run build && vitest",
	"start:test": "vitest",
	"test": "vitest run",

[karnthis]

run build is required to capture changes to the codebase because tests run specifically against the files in /dist, and without it the test system will only reflect changes to the tests themselves. test:build forces a build of the codebase to ensure all tests run successfully against the latest code.

if there is another way to capture code changes in list I would love to use it because this honestly isn't great.

[Rycochet]

Good point - it still needs that ternary for the moment then - I'm about to be pushing a test update that tests the built cjs (currently) via the new commandline tool and a bash script (the intention is that other implementations can support the same arguments and then run the same tests) - currently I can't get nodejs to load binary data properly though, so raw and uint8array both fail to decode :-(

[Rycochet]

All the test code has been updated and fixed, so it works properly - the test strategy will be changing slightly for the production code so it'll only run when it can (and force it on github itself) - so I'll clean up this when I get to it - can undo the change in the meantime :-)

[Rycochet]

This can now be removed - the test stategy has been updated (including better tests for built code, and an external test tool here - I'll get it running in github actions soon!)

[karnthis]

@Rycochet your thoughts, if you would please.

[Rycochet]

Typedocs to be added, and use generics rather than any - but abstraction is good :-)

[karnthis]

the reason for the any is because there are 2 incompatible function signatures, and I haven't been able to find a way to reconcile them. do you have any suggestions?

examples:
compress: (input: string | null) => T | null and (input: string) => T
decompress: (input: T | null) => string | null and (input: T) => string

[Rycochet]

I'm sure it's possible, but might be worth merging this in and fixing later

[Rycochet]

Cache whether the notice has been written for this function before, otherwise it can spam the console.

I'd actually suggest making this into a decorator instead which makes the code "cleaner" - it also means you can replace the function call with the original after the first call so no need to cache it (hmu if you need to!)

I've opened a poll on #190 which will decide whether to do this style, or fix them and provide legacy endpoints (I'm leaning towards this personally, and it's the more "lz-string" way, but if we get a strong opinion another way I'll happily go with it)

[karnthis]

if you could pm or email me that would be great, I'm not actually sure what you are suggesting.

[Rycochet]

I started to write a reply and go through the various ways it could be done (as we can't use a real typescript / javascript decorator outside of a class) - and realised that it's premature optimisation and we can worry about this later - plus it's far more useful for developers than for the end-user, so we've got until the final release of v2 to improve matters, with this as a good place to start from :-)

[Rycochet]

Almost there I think :-D

[Rycochet]

Should merge into one import (format might want to play though!)

Suggested change

	import { compressToBase64, compressToBase64_fixed } from ".";
	import { decompressFromBase64, decompressFromBase64_fixed } from ".";
	import {
	compressToBase64,
	compressToBase64_fixed,
	decompressFromBase64,
	decompressFromBase64_fixed,
	} from ".";

[Rycochet]

I think this is good to merge once updated and tests are run :-)

[karnthis]

I have everything done except the tests. since you redid the library I am having issues as my tests no longer work but the code being tested did not change. @Rycochet can you go into details on your process for setting up the base64 tests, since that is very similar to what the new tests need?

[Rycochet]

@karnthis The biggest thing is that the tests will fail when there is a fix to the underlying code - they're comparing against a "known good" binary output, but in reality the fix means that's not correct any more - you should be able to delete the files for the two fixed encoders, and re-run the test.sh script with the -u option to update those files - which will then be used by the actual vitest tests - do feel free to update things and push with those tests still failing if it's not clear (and then I can checkout locally and give any other pointers that might be needed) :-)

[Rycochet]

(Note that once this is merged I want to add specific handling for the legacy versions into the test scripts - but not entirely sure what form that needs to take!)

[ghost]

Deny

[karnthis]

considering the significant number of changes that have happened to the master branch since this started I am considering scrapping what is here and starting fresh. The work still needs to be done but it will need to be approached differently now that things have stabilized a bit.

@Beeloo0011 what?

[wkrick]

Beeloo0011 what?

@karnthis

I think this is a bot. Their GitHub history is really bizarre. Like they're probing for insecure repos or something.

[karnthis]

yeah that was my assessment too, was curious to see if I would get a response. didn't expect someone else to reply lol.

[karnthis]

@Rycochet