Issues and v2 work updates #189

karnthis · 2024-01-17T07:44:38Z

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

List to be updated as work is completed.

update local from source

src/utils/misc.ts

Rycochet · 2024-01-21T22:26:20Z

package.json

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


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",

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.

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 :-(

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 :-)

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 · 2024-01-25T02:27:28Z

@Rycochet your thoughts, if you would please.

src/__tests__/testFunctions.ts

Rycochet · 2024-01-25T09:20:44Z

src/__tests__/testFunctions.ts

@@ -39,6 +39,39 @@ export function getTestData(name: string) {
    return cachedTestData[name] || (cachedTestData[name] = readFileSync(`testdata/${name}/data.bin`).toString());
 }

+// eslint-disable-next-line  @typescript-eslint/no-explicit-any
+function runGeneralTests(identifier: string, compressFunc: (input: any) => any, decompressFunc: (input: any) => any) {


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

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

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

Rycochet · 2024-01-25T09:26:53Z

src/utils/misc.ts

@@ -0,0 +1,9 @@
+type VERSIONS = "v2.0.0";
+
+export function deprecated(thing: string, version: VERSIONS, opts?: { replacement?: string }): void {


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)

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

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 :-)

src/base64/compressToBase64URL.ts

src/base64/compressToBetterBase64.ts

src/base64/decompressFromBetterBase64.ts

src/__tests__/testFunctions.ts

Rycochet · 2024-01-25T09:35:35Z

Almost there I think :-D

Rycochet · 2024-01-26T19:24:15Z

src/base64/base64.test.ts

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


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 · 2024-02-05T23:00:29Z

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 · 2024-02-05T23:08:56Z

@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 · 2024-02-05T23:09:56Z

(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!)

Beeloo0011

Deny

karnthis · 2024-05-30T19:46:16Z

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 · 2024-05-30T20:36:48Z

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 · 2024-05-30T20:38:56Z

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 · 2024-10-09T23:50:49Z

@Rycochet

karnthis and others added 4 commits January 16, 2024 23:52

Merge pull request #1 from pieroxy/master

ce476fe

update local from source

Chore: address invalid Base64 returns (/pieroxy/issues/184)

a1e8dd0

Chore: address invalid Base64 encoding (/pieroxy/issues/110)

aec06d2

Chore: implements Base64URL suggestion (/pieroxy/pull/127)

32e6ed1

Rycochet reviewed Jan 17, 2024

View reviewed changes

src/utils/misc.ts Outdated Show resolved Hide resolved

karnthis and others added 5 commits January 20, 2024 19:07

removing browser-specific elements from utils

b8f6898

tests pt.1 - splitting into reusable library

c0eeb08

tests pt.2 - breaking out test sets to allow for variations

e6c2537

tests pt.3 - started creating more specific tests. all tests now pass

4b030c6

tests pt.4 - safer test running

42d7a24

Rycochet reviewed Jan 21, 2024

View reviewed changes

karnthis added 3 commits January 23, 2024 00:08

Merge branch master 'pieroxy/master' into resync

6e33f3a

refactored updates

bde410d

formatting

89a6f53

karnthis marked this pull request as ready for review January 25, 2024 02:27