Skip to content
This repository was archived by the owner on Oct 21, 2022. It is now read-only.

fix #20 - add a workaround for undefined export var #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix #20 - add a workaround for undefined export var #22

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 node/ltnodeclient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,12 @@ function sbRequire(require, resolve, path) {
if(!getSB(path)) {
if(path.match(/[\\\/]/) && fpath.extname(path) == ".js") {
var sb = sandboxes[path.toLowerCase()] = context(path);
Copy link
Author

@rbecheras rbecheras Nov 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so... But its coding style, so it's about conventions so my opinion doesn't matter. at all.

If the maintainers say to do this that way, sure I will.

But if you take a look at this project coding style, and especially for this file, it seems it's my way that is applied.

Javascript/node/ltnodeclient.js

Line 7 in 4d0f882

var util = require("util");

Copy link
Contributor

Choose a reason for hiding this comment

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

For what it is worth, I am not aware of anyone actively maintaining this plugin. So, unless another Light Table maintainer says otherwise, I'll leave it up to you.

Copy link
Author

Choose a reason for hiding this comment

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

Ok so, who is able to merge the PRs ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, to clarify, anyone on the Light Table team can review and merge it.

My comment was simply to suggest it is not likely that any of us will tell you to change the coding style.

Copy link
Author

@rbecheras rbecheras Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@sbauer322 Ok. Thanks.
And so.. Do you need a maintainer for this repo? I would be happy to join you as maintainer for the JavaScript plugin or even for the main or the other side projects. I search to invest myself in OSS joining maintainer teams and GitHub organizations for the projects I love. And Light Table is becoming one of them ;-)

Copy link
Author

Choose a reason for hiding this comment

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

@Mouvedia If you create an issue for your feature request, mention me with the arobase symbol, so that I'll get notified

Copy link
Contributor

@sbauer322 sbauer322 Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

A number of people desire a better JavaScript experience with Light Table and I think it would be great to have an active maintainer for this plugin (and other JS related ones). You also seem to have a long history of contributing to open source, which is beneficial.

My main concern is if you are familiar, or at least willing to learn, Clojure or ClojureScript? Being an expert in them wouldn't be necessary, but at least enough to deal with the plugin's code. Both Light Table and some of this plugin are written in them. Aside from this, I don't have any qualms... @rundis , @cldwalker - any concerns or thoughts?

This pull request is likely not the proper location to have this discussion, but perhaps our Gitter would be better.

Copy link

Choose a reason for hiding this comment

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

@rbecheras #24

Copy link
Contributor

Choose a reason for hiding this comment

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

@rbecheras - what would the simplest way to reproduce the error to make sure this PR fixes it? This linked issue, LightTable/LightTable#2146, gave some steps... should that suffice?

Copy link

Choose a reason for hiding this comment

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

大哥能帮我翻译一下评论吗😉😉

var _cleanCode = cleanCode(fs.readFileSync(path));
_cleanCode = ';var exports = module.exports;' + _cleanCode;
moduleCache[path] = sb.module;
prevModule = currentModule;
currentModule = sb.module;
vm.runInContext(cleanCode(fs.readFileSync(path)), sb, path);
vm.runInContext(_cleanCode, sb, path);
currentModule = prevModule;
sb.module.loaded = true;
} else {
Expand Down