edwindj / daff

Diff, patch and merge for data.frames, see http://paulfitz.github.io/daff/
https://edwindj.github.io/daff/
Other
153 stars 18 forks source link

e is locked #1

Closed timelyportfolio closed 9 years ago

timelyportfolio commented 9 years ago

When I run, I get

Error in e$ctx <<- V8::new_context("window") : 
  cannot change value of locked binding for 'e'

Should you use assign instead?

e <- new.env()

get_context <- function(){
  if (is.null(e$ctx)){
    assign("ctx",V8::new_context("window"),envir=e )
    e$ctx$source(system.file("js/underscore.js", package="V8"))
    e$ctx$source(system.file("js/daff.js", package="daff"))
    e$ctx$source(system.file("js/util.js", package="daff"))
  }
  e$ctx
}

or, should e be a list rather than environment like this?

e <- list()

get_context <- function(){
  if (is.null(e$ctx)){
    e$ctx <- V8::new_context("window")
    e$ctx$source(system.file("js/underscore.js", package="V8"))
    e$ctx$source(system.file("js/daff.js", package="daff"))
    e$ctx$source(system.file("js/util.js", package="daff"))
  }
  e$ctx
}
edwindj commented 9 years ago

Thanks! I'm not sure if we should cache the context at all, but may give better performance. Will look into it shortly.

edwindj commented 9 years ago

Fixed the issue; case of premature optimization...

timelyportfolio commented 9 years ago

Definitely works now, but I wonder (like you) if we would want to preserve/cache the context. Haven't done any packages that use V8, so not sure. I wonder if @jeroenooms could help add insight.

jeroen commented 9 years ago

You can only assign/change objects in the package namespace in the .onLoad function, before the namespace gets locked. But I think what you're doing now (using a separate context for each instance) is usually better, unless you're using some huge js libs which take a long time to load. The new rjade package does both.