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

BrowserFS implementation #24

Closed
scenaristeur opened this issue Jul 20, 2020 · 8 comments
Closed

BrowserFS implementation #24

scenaristeur opened this issue Jul 20, 2020 · 8 comments
Labels
enhancement New feature or request

Comments

@scenaristeur
Copy link

Hi guys, running opening tests/browser-test.html in the browser, i got a TypeError: can't access property "basename", u is undefined

image

image

and same with my webapp

image

Do you have any idea where it comes from ?
THxs

@Otto-AA
Copy link
Contributor

Otto-AA commented Jul 20, 2020

Hi guys

...and girls, and men and women and transgender and non-binaries, etc. (or simply "Hey there, running..." / "Running..."). Just a little reminder for inclusiveness :)

The problem is in rest.js. There we define a path variable which uses the default path library for the file://... scheme and the path.posix library for other schemes (app://ls/..., etc). The idea is, that we need OS-specific paths if we use the current file system, but for localStorage we should use a common one (posix in this case).

Here's the relevant part of the fetch function: https://github.com/jeff-zucker/solid-rest/blob/9567fb77abed828174c5136f9c82aa46e49d5743/src/rest.js#L48-L60

This should work fine, but the problem is that the other methods don't have access to this local path variable, hence it is undefined for them. For instance here in the getHeaders function: https://github.com/jeff-zucker/solid-rest/blob/9567fb77abed828174c5136f9c82aa46e49d5743/src/rest.js#L240-L243

So a simple solution would be to store the path as a field variable of the SolidRest instance (this.rest = path) and then use this.

@scenaristeur
Copy link
Author

scenaristeur commented Jul 20, 2020

Sorry @Otto-AA , i don't want to bless anyone.
Do you plan to update https://github.com/jeff-zucker/solid-rest/blob/master/tests/browser-test.html ?
I'm not sure where i have to add it in the example

async function runRest(file,text){
  file = "app://ls"+file
  const rest = new SolidRest([ new SolidLocalStorage() ])
  let response = await rest.fetch( file,{method:"PUT",body:text} )
  response = await rest.fetch( file )
  show(response.status+","+await response.text())
}

Thxs all ;-)

@Otto-AA
Copy link
Contributor

Otto-AA commented Jul 30, 2020

I think @jeff-zucker fixed most of this in this commit: 9e60683

But I don't think that this will work, because options is likely undefined here (as far as I see it's a local variable in the fetch method and it is not passed as parameter to _getExtension ):
https://github.com/jeff-zucker/solid-rest/blob/9e60683490aa4d298eca0ee99eb164302e5b198c/src/rest.js#L217-L224

To fix this you can either add the options to the function declaration (e.g. function _getExtension(pathname, options) ) or assign it to this instead of the options object.


In general to make the code cleaner I'd suggest to split SolidRest into the two classes SolidRest and RequestHandler. The RequestHandler constructor could take the url, storage and options as parameter. The benefit would be, that you could assign the options to this from the RequestHandler and then access it from the methods without passing it as parameter. That would make it easier to split the fetch method it into smaller functions.

So it would look something like this:

class SolidRest {
constructor( handlers ) {
  this.storageHandlers = {}
  if( typeof handlers ==="undefined") return
  handlers.forEach( handler => {
     console.log(`
       Installing Rest Handler ${handler.name} using prefix ${handler.prefix}
     `)
     this.storageHandlers[handler.prefix] = handler
  })
}

getStorage(uri){
  const { protocol } = new URL(uri)
  const prefix = protocol // TODO
  if(!this.storageHandlers[prefix]) throw "Did not recognize prefix "+prefix
  return this.storageHandlers[prefix]
}

async fetch(uri, options = {}) {
  const storage = this.getStorage(uri)
  const requestHandler = new RequestHandler(uri, storage, options)
  return requestHandler.fetch()
}
}

class RequestHandler {
  constructor(uri, storage, options) {
    this.uri = uri
    this.storage = storage
    this.options = options
  }

  async fetch() {
    switch(this.options.method) {
      case 'GET':
        return this.get()
      case 'POST':
        return this.post()
      // ...
  }
  // ...
}

If you want a refactor like this I could make a PR for it. It's not necessary, but it should make the code easier to maintain.

@scenaristeur
Copy link
Author

Hi Thxs, in fact I don't really know what I want... I just want to understand how to use solid-rest. 😉
My goal is to have from a webapp a Solid like storage on my Android phone...
I think localstorage is not so good as it is not persistent, so I was looking for a system file storage or something like https://web.dev/persistent-storage/
I know solid-rest can be configured with many browserFs plugin, so I'm looking for an example to store/retrieve data on a local-persistant-solid-like Pod...

@jeff-zucker
Copy link
Member

@scenaristeur I'm afraid I need to concentrate on the file:/// part of solid-rest and will not be able to even provide adequate examples of browserFS. I will probably mark it as experimental in the documentation. Nevertheless I believe that it can (or with a few tweaks it could) do what you want using either indexedDB or the Native File API, both of which are permenent and can be accessed via BrowserFS. If you run into specific issues, please let me know but I can't promise much response until other module issues are taken care of.

@jeff-zucker jeff-zucker changed the title browser-test BrowserFS implementation Jul 31, 2020
@jeff-zucker jeff-zucker added the enhancement New feature or request label Jul 31, 2020
@jeff-zucker
Copy link
Member

@Otto-AA wrote : To fix this you can either add the options to the function declaration (e.g. function _getExtension(pathname, options) ) or assign it to this instead of the options object.

I went with making options available everywhere it is needed. Using this would probably be cleaner, but will have to wait for another time.

Otto and @bourgeoa - I've prioritized the issues as I see them and want to fix the red and orange ones before the green ones. If you would prioritize differently, let me know.

@jeff-zucker
Copy link
Member

@scenaristeur - I take it back, IT WORKS!. The fix turned out to be easier than I thought. It involves adding an initBackends call. I've re-written the browser-test.html and included some documentation in it. So you should be able to permanently store resources and containers in your Android browser's Native File API (HTML5FS) or IndexedDB or other storage spaces supported by BrowserFS. I haven't npm'd it yet, but the current 1.22 master has all the changes.

@Otto-AA - it was good you caught the path/basename problem but LOL the error was from basename in the BrowserFS package, not in anything in solid-rest or solid-rest's browserFS plugin.

It turns out that @CxRes's fix of the path/path.posix issue doesn't work in the browser - path.posix is undefined, so now I default to path if there is no path.posix.

@scenaristeur
Copy link
Author

Thxs , I'll give it a try soon 👍😘😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants