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

_splitPath function currently prevents the use of URL's as document identifiers (or any identifiers with . in) #289

Open
wip-abramson opened this issue Jun 3, 2021 · 7 comments

Comments

@wip-abramson
Copy link

We have been attempting to use racer with ShareDb and wish to use URL identifiers for our models so they can be represented as JSON-LD compatible data models.

We ran into some challenges when attempting to use the at() or scope() functions to access these documents. Note: they appear to get created without issue.

After some digging I identified the problem in the _splitPath function see

Model.prototype._splitPath = function(subpath) {

Currently this splits on "." so all URL identifier will cause 3 items to be placed in the list instead of the expected two causing an error to be thrown.

Is it possible to only look for and split on the first "." in the string?

@wip-abramson
Copy link
Author

This seems more complicated than I anticipated, since it seems the subpath can be any number of arguments chained with.

Still looking to add support for URL identifiers.

Any reason why this is not possible to support, other than relatively substantial code refactor? I would be grateful for any pointers.

Thanks

@jandrieu
Copy link

This is fairly vital for a use case we have, which is based on semantic web data models where, essentially, all of our ids are URIs.

We'd be willing to submit a PR that abstracts out the splitPath functionality, but we'd like to get a sense of whether or not that will be a welcome improvement.

@ericyhwang
Copy link
Contributor

Hello!

The biggest thing blocking support of dot literals in paths is that currently, the internal representation of a child model's location in the data tree is Model#_at, a dot-separated string.

A year or so ago, I did think about refactoring the internal code to instead use an array of path segments. I mostly wanted to lower the overhead of having to splitting and joining strings, but it would also allow adding support for dot literals in path segments.

Unfortunately, I ended up realizing I didn't have the time to do the refactor and thoroughly run performance tests comparing the two versions, which is important because path manipulations happen a lot in a Derby app.

Fully supporting dot literals also gets tricky because, while the references to model._at are mostly limited to the small paths.js file, there's other features of Racer (event listeners, reactive functions, refs, etc.) that call model.path() to get an absolute path-string for use in those features.

That said, if you happen to not use those features, partial support of dot literals probably wouldn't be too bad as long as performance checks out ok, if you're fine with potential bugs in events and such until those are updated.

@ericyhwang
Copy link
Contributor

What I was thinking "partial support, possible bugs" would look like:

  • at() and scope() would support taking the input path as an array of path segments, also keeping support for today's path string
  • Internally, add model._atSegments, an array of path segments, as the new source of truth, with model._at kept in place as a cache for backwards compatibility on model.path() calls. _atSegments should never be directly returned via any external API, since if an API consumer inadvertently modifies it in-place, Racer will get very confused.

@jandrieu
Copy link

@ericyhwang That sounds reasonable.

Is there any current code for testing performance? We've checked out the unit tests, but at least for path that looked like it was just conformance, not performance.

@craigbeck
Copy link
Contributor

Thanks for your contributing your issue.

We have recently been actively updating the Derby and Racer packages, and both repos are now in Typescript and published with types on npm. As we have quite a few old issues open we have made the decision to close out of date issues.

If this issue still matters to you we encourage reproducing against the current versions of the repo and opening a new issue.

@ericyhwang
Copy link
Contributor

Reopening since this is something I still want to do at some point, and this has discussion and context already. I don't currently have time scheduled out to work on it, though.

Now that the codebase is in TS, it should be safer to refactor _at into an array of segments, at least!

@ericyhwang ericyhwang reopened this May 30, 2024
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

No branches or pull requests

4 participants