-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Update index.js #8619
Conversation
minor code fixes
There was a problem hiding this 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
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? |
@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. |
Help me understand... how is |
@garethbowen that alone does not do help when you not see the rest of the code the trick here is
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. |
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 |
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! |
@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 |
A nice idea 👍 I tinker with something similar in my head since this spring. |
@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. |
@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. |
@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 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. |
minor code fixes