Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

Binding: Refurbishes binding to use new API #543

Merged
merged 9 commits into from
Dec 18, 2014
Merged

Binding: Refurbishes binding to use new API #543

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
af72ccc
Binding: Refurbishes binding to use new API.
am11 Nov 16, 2014
739cc08
Code: Cleanup.
am11 Nov 17, 2014
4abc7dd
Code: Mitigates chances of memory leaks.
am11 Nov 17, 2014
9e436db
Binding: Fixes for stats.
am11 Nov 17, 2014
a8c6710
Code: Brings more optimizations.
am11 Nov 17, 2014
9f7a78d
Errors: Convert errors to JS objects.
am11 Nov 18, 2014
d58c303
Options: Adds options for source-maps varieties.
am11 Nov 18, 2014
67ea9c5
Source-map: Parse as JSON.
am11 Nov 19, 2014
187cfde
Code: Fixes condition is-output-path-relative.
am11 Nov 19, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion binding.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@
'src/libsass/remove_placeholders.cpp',
'src/libsass/sass.cpp',
'src/libsass/sass2scss.cpp',
'src/libsass/sass_interface.cpp',
'src/libsass/sass_context.cpp',
'src/libsass/sass_functions.cpp',
'src/libsass/sass_util.cpp',
'src/libsass/sass_values.cpp',
'src/libsass/source_map.cpp',
'src/libsass/to_c.cpp',
'src/libsass/to_string.cpp',
Expand Down
198 changes: 110 additions & 88 deletions src/binding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,6 @@
#include <nan.h>
#include <string>
#include <cstring>
#include <iostream>
#include <cstdlib>
#include "sass_context_wrapper.h"

using namespace v8;
using namespace std;

void WorkOnContext(uv_work_t* req) {
sass_context_wrapper* ctx_w = static_cast<sass_context_wrapper*>(req->data);
if (ctx_w->ctx) {
sass_context* ctx = static_cast<sass_context*>(ctx_w->ctx);
sass_compile(ctx);
} else if (ctx_w->fctx) {
sass_file_context* ctx = static_cast<sass_file_context*>(ctx_w->fctx);
sass_compile_file(ctx);
}
}

char* CreateString(Local<Value> value) {
if(value->IsNull() || !value->IsString()) {
return const_cast<char*>(""); // return empty string.
Expand All @@ -30,9 +12,8 @@ char* CreateString(Local<Value> value) {
return str;
}

void ExtractOptions(Local<Value> optionsValue, void* cptr, sass_context_wrapper* ctx_w, bool isFile) {
void ExtractOptions(Local<Object> options, void* cptr, sass_context_wrapper* ctx_w, char* data_or_path, bool isFile) {
bool source_comments;
Local<Object> options = optionsValue->ToObject();

if (ctx_w) {
NanAssignPersistent(ctx_w->stats, options->Get(NanNew("stats"))->ToObject());
Expand All @@ -41,66 +22,76 @@ void ExtractOptions(Local<Value> optionsValue, void* cptr, sass_context_wrapper*
Local<Function> callback = Local<Function>::Cast(options->Get(NanNew("success")));
Local<Function> errorCallback = Local<Function>::Cast(options->Get(NanNew("error")));
if (isFile) {
ctx_w->fctx = (sass_file_context*) cptr;
ctx_w->fctx = (struct Sass_File_Context*) cptr;
} else {
ctx_w->ctx = (sass_context*) cptr;
ctx_w->dctx = (struct Sass_Data_Context*) cptr;
}
ctx_w->request.data = ctx_w;
ctx_w->callback = new NanCallback(callback);
ctx_w->errorCallback = new NanCallback(errorCallback);
}

if (isFile) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably merge most or all of the following statements, as they now share a common struct (Sass_Options) to setup the options (before you had to split it up, since the options were living on two totally unrelated structs).

sass_file_context* ctx = (sass_file_context*) cptr;
ctx->input_path = CreateString(options->Get(NanNew("file")));
ctx->output_path = CreateString(options->Get(NanNew("outFile")));
ctx->options.image_path = CreateString(options->Get(NanNew("imagePath")));
ctx->options.output_style = options->Get(NanNew("style"))->Int32Value();
ctx->options.source_comments = source_comments = options->Get(NanNew("comments"))->BooleanValue();
ctx->options.omit_source_map_url = options->Get(NanNew("omitSourceMapUrl"))->BooleanValue();
ctx->options.source_map_file = CreateString(options->Get(NanNew("sourceMap")));
ctx->options.include_paths = CreateString(options->Get(NanNew("paths")));
ctx->options.precision = options->Get(NanNew("precision"))->Int32Value();
struct Sass_File_Context* fctx = (struct Sass_File_Context*) cptr;
struct Sass_Context* ctx = sass_file_context_get_context(fctx);
struct Sass_Options* sass_options = sass_context_get_options(ctx);

sass_option_set_input_path(sass_options, data_or_path);
sass_option_set_output_path(sass_options, CreateString(options->Get(NanNew("outFile"))));
sass_option_set_image_path(sass_options, CreateString(options->Get(NanNew("imagePath"))));
sass_option_set_output_style(sass_options, (Sass_Output_Style)options->Get(NanNew("style"))->Int32Value());
sass_option_set_source_comments(sass_options, source_comments = options->Get(NanNew("comments"))->BooleanValue());
sass_option_set_omit_source_map_url(sass_options, options->Get(NanNew("omitSourceMapUrl"))->BooleanValue());
sass_option_set_source_map_file(sass_options, CreateString(options->Get(NanNew("sourceMap"))));
sass_option_set_include_path(sass_options, CreateString(options->Get(NanNew("paths"))));
sass_option_set_precision(sass_options, options->Get(NanNew("precision"))->Int32Value());
} else {
sass_context* ctx = (sass_context*) cptr;
ctx->source_string = CreateString(options->Get(NanNew("data")));
ctx->output_path = CreateString(options->Get(NanNew("outFile")));
ctx->options.is_indented_syntax_src = options->Get(NanNew("indentedSyntax"))->BooleanValue();
ctx->options.image_path = CreateString(options->Get(NanNew("imagePath")));
ctx->options.output_style = options->Get(NanNew("style"))->Int32Value();
ctx->options.source_comments = source_comments = options->Get(NanNew("comments"))->BooleanValue();
ctx->options.omit_source_map_url = options->Get(NanNew("omitSourceMapUrl"))->BooleanValue();
ctx->options.source_map_file = CreateString(options->Get(NanNew("sourceMap")));
ctx->options.include_paths = CreateString(options->Get(NanNew("paths")));
ctx->options.precision = options->Get(NanNew("precision"))->Int32Value();
struct Sass_Data_Context* dctx = (struct Sass_Data_Context*) cptr;
struct Sass_Context* ctx = sass_data_context_get_context(dctx);
struct Sass_Options* sass_options = sass_context_get_options(ctx);

sass_option_set_output_path(sass_options, CreateString(options->Get(NanNew("outFile"))));
sass_option_set_is_indented_syntax_src(sass_options, options->Get(NanNew("indentedSyntax"))->BooleanValue());
sass_option_set_image_path(sass_options, CreateString(options->Get(NanNew("imagePath"))));
sass_option_set_output_style(sass_options, (Sass_Output_Style)options->Get(NanNew("style"))->Int32Value());
sass_option_set_source_comments(sass_options, source_comments = options->Get(NanNew("comments"))->BooleanValue());
sass_option_set_omit_source_map_url(sass_options, options->Get(NanNew("omitSourceMapUrl"))->BooleanValue());
sass_option_set_source_map_file(sass_options, CreateString(options->Get(NanNew("sourceMap"))));
sass_option_set_include_path(sass_options, CreateString(options->Get(NanNew("paths"))));
sass_option_set_precision(sass_options, options->Get(NanNew("precision"))->Int32Value());
}
}

template<typename Ctx>
void FillStatsObj(Handle<Object> stats, Ctx ctx) {
int i;
Handle<Array> arr;
void FillStatsObj(Handle<Object> stats, Sass_Context* ctx) {
char** included_files = sass_context_get_included_files(ctx);
//int length = sizeof(included_files) / sizeof(included_files[0]);
//int length = std::char_traits<char*>::length(included_files);
Handle<Array> arr = NanNew<Array>();

arr = NanNew<Array>(ctx->num_included_files);
for (i = 0; i < ctx->num_included_files; i++) {
arr->Set(i, NanNew<String>(ctx->included_files[i]));
if(included_files) {
for (int i = 0; included_files[i] != nullptr; ++i) {//cout<<included_files[i]<<endl;
arr->Set(i, NanNew<String>(included_files[i]));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be cleaned up: remove comments.

}
}

(*stats)->Set(NanNew("includedFiles"), arr);
}

void FillStatsObj(Handle<Object> stats, sass_file_context* ctx) {
void FillStatsObj(Handle<Object> stats, struct Sass_File_Context* fctx) {
Handle<Value> source_map;
struct Sass_Context* ctx = sass_file_context_get_context(fctx);

FillStatsObj<sass_file_context*>(stats, ctx);
FillStatsObj(stats, ctx);

if (ctx->error_status) {
if (sass_context_get_error_status(ctx)) {
return;
}
if (ctx->source_map_string) {
source_map = NanNew<String>(ctx->source_map_string);
if (sass_context_get_source_map_string(ctx)) {
source_map = NanNew<String>(sass_context_get_source_map_string(ctx));
} else {
source_map = NanNull();
}

(*stats)->Set(NanNew("sourceMap"), source_map);
}

Expand All @@ -109,25 +100,30 @@ void MakeCallback(uv_work_t* req) {

TryCatch try_catch;
sass_context_wrapper* ctx_w = static_cast<sass_context_wrapper*>(req->data);
int error_status = ctx_w->ctx ? ctx_w->ctx->error_status : ctx_w->fctx->error_status;
int error_status;
struct Sass_Context* ctx;

if (ctx_w->ctx) {
FillStatsObj(NanNew(ctx_w->stats), ctx_w->ctx);
if (ctx_w->dctx) {
ctx = sass_data_context_get_context(ctx_w->dctx);
FillStatsObj(NanNew(ctx_w->stats), ctx);
error_status = sass_context_get_error_status(ctx);
} else {
FillStatsObj(NanNew(ctx_w->stats), ctx_w->fctx);
ctx = sass_file_context_get_context(ctx_w->fctx);
FillStatsObj(NanNew(ctx_w->stats), ctx);
error_status = sass_context_get_error_status(ctx);
}

if (error_status == 0) {
// if no error, do callback(null, result)
char* val = ctx_w->ctx ? ctx_w->ctx->output_string : ctx_w->fctx->output_string;
const char* val = sass_context_get_output_string(ctx);
Local<Value> argv[] = {
NanNew<String>(val),
NanNew(ctx_w->stats)->Get(NanNew("sourceMap"))
};
ctx_w->callback->Call(2, argv);
} else {
// if error, do callback(error)
char* err = ctx_w->ctx ? ctx_w->ctx->error_message : ctx_w->fctx->error_message;
const char* err = sass_context_get_error_json(ctx);
Local<Value> argv[] = {
NanNew<String>(err),
NanNew<Integer>(error_status)
Expand All @@ -137,77 +133,103 @@ void MakeCallback(uv_work_t* req) {
if (try_catch.HasCaught()) {
node::FatalException(try_catch);
}

sass_free_context_wrapper(ctx_w);
}

NAN_METHOD(Render) {
NanScope();

sass_context* ctx = sass_new_context();
sass_context_wrapper* ctx_w = sass_new_context_wrapper();
ctx_w->ctx = ctx;
ExtractOptions(args[0], ctx, ctx_w, false);
Local<Object> options = args[0]->ToObject();
char* source_string = CreateString(options->Get(NanNew("data")));
struct Sass_Data_Context* dctx = sass_make_data_context(source_string);
sass_context_wrapper* ctx_w = sass_make_context_wrapper();

ctx_w->dctx = dctx;

ExtractOptions(options, dctx, ctx_w, source_string, false);

int status = uv_queue_work(uv_default_loop(), &ctx_w->request, compile_it, (uv_after_work_cb)MakeCallback);

int status = uv_queue_work(uv_default_loop(), &ctx_w->request, WorkOnContext, (uv_after_work_cb)MakeCallback);
assert(status == 0);
// free(source_string);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgreter, is this necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

source_string will be freed once the context is destroyed (since it is added to sources).
So yes, this free can be removed (IMHO there is no chance it wont get added to context).


NanReturnUndefined();
}

NAN_METHOD(RenderSync) {
NanScope();
Handle<Object> options = args[0]->ToObject();
sass_context* ctx = sass_new_context();
ExtractOptions(args[0], ctx, NULL, false);

sass_compile(ctx);
Local<Object> options = args[0]->ToObject();
char* source_string = CreateString(options->Get(NanNew("data")));
struct Sass_Data_Context* dctx = sass_make_data_context(source_string);
struct Sass_Context* ctx = sass_data_context_get_context(dctx);

ExtractOptions(options, dctx, NULL, source_string, false);
compile_data(dctx);
FillStatsObj(options->Get(NanNew("stats"))->ToObject(), ctx);
// free(source_string);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgreter, is this necessary?


if (sass_context_get_error_status(ctx) == 0) {
Local<String> output = NanNew<String>(sass_context_get_output_string(ctx));

if (ctx->error_status == 0) {
Local<String> output = NanNew<String>(ctx->output_string);
free_context(ctx);
free_data_context(dctx);
NanReturnValue(output);
}

Local<String> error = NanNew<String>(ctx->error_message);
free_context(ctx);
Local<String> error = NanNew<String>(sass_context_get_error_json(ctx));

free_data_context(dctx);
NanThrowError(error);

NanReturnUndefined();
}

NAN_METHOD(RenderFile) {
NanScope();
sass_file_context* fctx = sass_new_file_context();
sass_context_wrapper* ctx_w = sass_new_context_wrapper();

Local<Object> options = args[0]->ToObject();
char* input_path = CreateString(options->Get(NanNew("file")));
struct Sass_File_Context* fctx = sass_make_file_context(input_path);
sass_context_wrapper* ctx_w = sass_make_context_wrapper();

ctx_w->fctx = fctx;
ExtractOptions(args[0], fctx, ctx_w, true);
ExtractOptions(options, fctx, ctx_w, input_path, true);

int status = uv_queue_work(uv_default_loop(), &ctx_w->request, WorkOnContext, (uv_after_work_cb)MakeCallback);
int status = uv_queue_work(uv_default_loop(), &ctx_w->request, compile_it, (uv_after_work_cb)MakeCallback);
assert(status == 0);

// free(input_path);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgreter, is this necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this needs to stay as you create a copy with CreateString!?


NanReturnUndefined();
}

NAN_METHOD(RenderFileSync) {
NanScope();
sass_file_context* ctx = sass_new_file_context();
ExtractOptions(args[0], ctx, NULL, true);
Handle<Object> options = args[0]->ToObject();

sass_compile_file(ctx);
Local<Object> options = args[0]->ToObject();
char* input_path = CreateString(options->Get(NanNew("file")));
struct Sass_File_Context* fctx = sass_make_file_context(input_path);
struct Sass_Context* ctx = sass_file_context_get_context(fctx);

ExtractOptions(options, fctx, NULL, input_path, true);
compile_file(fctx);
FillStatsObj(options->Get(NanNew("stats"))->ToObject(), ctx);

if (ctx->error_status == 0) {
Local<String> output = NanNew<String>(ctx->output_string);
free_file_context(ctx);
// free(input_path);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgreter, is this necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this needs to stay as you create a copy with CreateString!?
And it should probably be outside the if statement (is not freed on errors?).

if (sass_context_get_error_status(ctx) == 0) {
Local<String> output = NanNew<String>(sass_context_get_output_string(ctx));

free_file_context(fctx);
NanReturnValue(output);
}

Local<String> error = NanNew<String>(ctx->error_message);
free_file_context(ctx);
Local<String> error = NanNew<String>(sass_context_get_error_json(ctx));

free_file_context(fctx);
NanThrowError(error);

NanReturnUndefined();
}

Expand Down
50 changes: 35 additions & 15 deletions src/sass_context_wrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,51 @@
#include "sass_context_wrapper.h"
#include <nan.h>
#include <cstdlib>

extern "C" {
using namespace std;

void free_context(sass_context* ctx) {
delete[] ctx->source_string;
delete[] ctx->options.include_paths;
delete[] ctx->options.image_path;
sass_free_context(ctx);
void compile_it(uv_work_t* req) {
sass_context_wrapper* ctx_w = static_cast<sass_context_wrapper*>(req->data);

if (ctx_w->dctx) {
compile_data(ctx_w->dctx);
} else if (ctx_w->fctx) {
compile_file(ctx_w->fctx);
}
}

void compile_data(struct Sass_Data_Context* dctx) {
struct Sass_Context* ctx = sass_data_context_get_context(dctx);
struct Sass_Options* ctx_opt = sass_context_get_options(ctx);
sass_compile_data_context(dctx);
}

void compile_file(struct Sass_File_Context* fctx) {
struct Sass_Context* ctx = sass_file_context_get_context(fctx);
struct Sass_Options* ctx_opt = sass_context_get_options(ctx);
sass_compile_file_context(fctx);
}

void free_data_context(struct Sass_Data_Context* dctx) {
// delete[] dctx->source_string;
// delete[] dctx->options.include_paths;
// delete[] dctx->options.image_path;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgreter, should we remove these?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should be safe to remove, since sass_free_xxx_context should take care of that!

sass_delete_data_context(dctx);
}

void free_file_context(sass_file_context* fctx) {
delete[] fctx->input_path;
delete[] fctx->options.include_paths;
delete[] fctx->options.image_path;
sass_free_file_context(fctx);
void free_file_context(struct Sass_File_Context* fctx) {
// delete[] fctx->input_path;
// delete[] fctx->options.include_paths;
// delete[] fctx->options.image_path;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgreter, should we remove these?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should be safe to remove, since sass_free_xxx_context should take care of that!

sass_delete_file_context(fctx);
}

sass_context_wrapper* sass_new_context_wrapper() {
sass_context_wrapper* sass_make_context_wrapper() {
return (sass_context_wrapper*) calloc(1, sizeof(sass_context_wrapper));
}

void sass_free_context_wrapper(sass_context_wrapper* ctx_w) {
if (ctx_w->ctx) {
free_context(ctx_w->ctx);
if (ctx_w->dctx) {
free_data_context(ctx_w->dctx);
} else if (ctx_w->fctx) {
free_file_context(ctx_w->fctx);
}
Expand Down
Loading