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

Update index.js #8619

Closed
wants to merge 1 commit into from
Closed

Update index.js #8619

wants to merge 1 commit into from

Conversation

lemanschik
Copy link

minor code fixes

minor code fixes
Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style change request.

allDocs(txn, metadata, opts, cb);
});

api._getAttachment = $t(getAttachment);
api["_getAttachment"] = $t(getAttachment);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this style is an improvement - why do you prefer this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is only a readability change to make it more easy to see where the code is that matters most parts of the code are relativ useless adapters as you maybe know this change allowed me to do faster code analyses as you used inheritance all over. If you want to keep this project alive which is far far far behind the official couchdb docs. then Style should not be the part to worry about...

but if you have a genuine interest to know if this style is a regression in terms of performance no it is not! https://chromium.googlesource.com/v8/v8.git/+/refs/heads/main/AUTHORS#120 i relativ sure know what i say at last i am familar with the v8 assembler and i am the creator of B8G the v8 Compiler feedback interface.

@lemanschik
Copy link
Author

mainly readability and it is the result of our internal compiler overall it was a fast shot to see if your open to changes and how fast the process would be.

I already had forked everything and a rewrite is ongoing as the current state is not useable.

@garethbowen
Copy link
Member

Ok. Well that change in particular makes this file inconsistent with the styling of other files so I think it's worse for readability (just my opinion - feel free to start a discussion with options). For now, would you be open to resubmitting the PR without the property access change?

@lemanschik
Copy link
Author

@garethbowen sure after but i did give you a short reply about that in your change request at the end my idea was to distinguish the main api parts this way so i can faster see and find parts that are not matching with the api in the docs.

@garethbowen
Copy link
Member

Help me understand... how is api["_getAttachment"] easier than api._getAttachment for distinguishing the main api?

@lemanschik
Copy link
Author

lemanschik commented Jun 12, 2023

@garethbowen that alone does not do help when you not see the rest of the code the trick here is

api["_getAttachment"] === original implementation api 1:1
api._getAttachment === api 1:X adapter like but same name

the above example shows the handy part you do not see the improvement in a single file but it makes also no sense to now make 1 gigantic PR so i started with a single file

this is called micro commits they allow us to slowly show what we want to do like now and then see if it gets adopted before we start to submit more changes.

@lemanschik
Copy link
Author

The final result of all this is a PuchDB rewrite as Readable Writeable and Transform Streams and removing most of the inheritance based abstractions replacing them with transform streams

@garethbowen
Copy link
Member

Micro commits are better done in vertical slices rather than horizontal, that is to say, change the definition of one api in multiple files, rather than changing the definition of all apis in one file. The approach you've taken reduces readability and provides no benefit until other files are changed.

Thank you for taking the time to explain your process!

@lemanschik
Copy link
Author

@garethbowen your correct with your comment about micro commits i am just overworked and i do not always do the best job at everything alone but that is value able input that i like a lot! Thanks for your input. My current state as sayed is already 100% rewritten. Even the Server parts and client parts are joined and API's are aligned with newst chromium also
using the OPFS for storage and as IndexedDB replacement

@SourceR85
Copy link
Contributor

The final result of all this is a PuchDB rewrite as Readable Writeable and Transform Streams and removing most of the inheritance based abstractions replacing them with transform streams

A nice idea 👍

I tinker with something similar in my head since this spring.
(manly to support SSE-Streams... until now I've no good idea how to implement it)

@lemanschik
Copy link
Author

@SourceR85 that are good news so i am now confident that i do not need to maintain that alone that is a good thing. i Will simple be careful and slowly replace parts and then you can jump in as soon as you understood the pattern.

It is a lot of small parts that need a bit of editing as the whole project got created in a time where no Native Implementations did exist.

@garethbowen
Copy link
Member

@lemanschik Are you proposing to drop indexeddb support, or provide another adapter for OPFS storage? The former would be difficult to merge as it would obviously break backwards compatibility with existing projects. However implementing an OPFS adapter would make it easy to compare both implementations and let projects pick the right storage system for them.

@lemanschik
Copy link
Author

lemanschik commented Jun 13, 2023

@garethbowen we do not brake any existing api while i did the rewrite i found some logic failures and other problems we will support all existing stuff but i will add a new outer layer
it offers a additional streams based api

const db = new ReadableStream({ ...mydb });
db.pipeThrough(new TransformStream({ ...processorsAndAdditionalInputLikeWrites })).pipeTo(
new WritableStream({ ...writeAndLogChangesAndSync })

When this outer layer is stalbe i will incrremental replace internal api's used in the old packages like fetch the nativ fetch for example

const db = await fetch().then(r=>r.body) // r.body === ReadableStream.

Under the hood all nodejs and web api's got aligned so for example fspromises takes reaadablestream directly we call that the iterable consumer pattern

const db = await fetch().then(r=>r.body) // r.body === ReadableStream.
fsPromises.writeFile('/path/db', db);

note that we did zero additional code writeFile takes all forms of iterables including webStreams.

the same wirteFile is do able with OPFS.

As also we finished the WASIX Standard this allows us to implement some high scale big data algos and so on.

i use this PouchDB as building Block of my own Quaternion DB Which aims to be a Couchbase Successor and the first of its kind net wide World Wide Linked in memory grid computer.

i liked the couchdb sync always and this was the most complete code base in ECMAScript.

as sayed i can imagin that we over time maybe depending on performance will even use WASM CouchDB under the hood and only keep the streams interface as api

but i will start now as i have general verification that you will help me to maintain that parts.

@lemanschik
Copy link
Author

closed by: #8661 #8598

@lemanschik lemanschik closed this Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants