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

e is locked #1

Closed
timelyportfolio opened this issue Feb 18, 2015 · 4 comments
Closed

e is locked #1

timelyportfolio opened this issue Feb 18, 2015 · 4 comments

Comments

@timelyportfolio
Copy link
Collaborator

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
Copy link
Owner

edwindj commented Feb 18, 2015

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

@edwindj
Copy link
Owner

edwindj commented Feb 18, 2015

Fixed the issue; case of premature optimization...

@edwindj edwindj closed this as completed Feb 18, 2015
@timelyportfolio
Copy link
Collaborator Author

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
Copy link
Contributor

jeroen commented Feb 18, 2015

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.

edwindj pushed a commit that referenced this issue Mar 30, 2017
Summarize using new upstream functionality & Further improve display
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

3 participants