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

WScript.GetDirectory() & WScript.GetFileName() #1399

Closed
wants to merge 1 commit into from

Conversation

Cellule
Copy link
Contributor

@Cellule Cellule commented Aug 6, 2016

Add WScript.GetFileName() and WScript.GetDirectory(), similar to __filename and __dirname in node.js.

Too often when trying to debug a file, you end up needing to either move to the script's directory or change your debugger to start in that script's directory.
Currently, there is no way to workaround this problem.

Will be able to do

WScript.LoadScriptFile(WScript.GetDirectory() + "../TestHarness.js"); 

Then run that script from anywhere.

Additionally, we could append the script's current directory to WScript.LoadScriptFile read readbuffer if it uses relative paths.


This change is Reviewable

@Cellule
Copy link
Contributor Author

Cellule commented Aug 8, 2016

@dotnet-bot test this please

@@ -5,6 +5,7 @@
JsAddRef
JsRelease
JsCreateContext
JsGetContextUrl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Exposing a new JSRT API? Is this intentional and API reviewed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only way for a ChakraCore host to get the url I guess? Probably want to run this by @curtisman if haven't already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes new API needs to be reviewed.

In this case, I don't think a new API is necessary. ch should be able to keep track of the script file name it is running, and get the filename and directory.
It is not the "current" script file, but it should be enough for what you need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll see what I can do. I just don't know what kind of information I can get in ch regarding the caller of a WScript method.
Since there can be multiple file after various WScript.LoadScriptFile(). Maybe I can make it that the methods returns the path of the initial script (ie from argv) and that might be good enough for the time being.

@Cellule
Copy link
Contributor Author

Cellule commented Aug 9, 2016

I used the existing APIs JsSetContextData and JsGetContextData instead.
Whenever we create a new jsrt context, we attach the path used of the script used for that context so we can refer to it later.
This will not work when using WScript.LoadScriptFile("myfile.js", "self") because we don't create a new context in this case.

@Cellule Cellule force-pushed the __dirname branch 4 times, most recently from 1114c05 to f3ce76a Compare August 11, 2016 00:49
@Cellule
Copy link
Contributor Author

Cellule commented Aug 11, 2016

I had to save the path before lowering the case because paths are case sensitive on Unix systems.

@Cellule
Copy link
Contributor Author

Cellule commented Aug 16, 2016

Ping

@@ -402,13 +405,23 @@ HRESULT CreateAndRunSerializedScript(const char* fileName, LPCSTR fileContents,
IfJsErrorFailLog(ChakraRTInterface::JsGetCurrentContext(&current));
IfJsErrorFailLog(ChakraRTInterface::JsSetCurrentContext(context));

// canonicalize that path name to lower case for the profile storage
// REVIEW: Assuming no utf8 characters here
Copy link
Collaborator

Choose a reason for hiding this comment

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

how much effort would it be to handle unicode characters? this seems like it could easily bite someone in the future, particularly folks in non-english speaking regions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see this is already a review comment from below elsewhere in the code. I suppose it hasn't been reported as a problem yet then and is okay to leave for now.

…ce when trying to execute a file that needs to refer to another file with a relative path.

Will be able to do
```
WScript.LoadScriptFile(WScript.GetDirectory() + "../TestHarness.js");
```
@Cellule
Copy link
Contributor Author

Cellule commented Aug 26, 2016

I rebased my change and remove the change for linux build script. When I'll have some more time I'll open another issue for that purpose.

@dilijev dilijev added this to the Backlog milestone Dec 20, 2016
@dilijev
Copy link
Contributor

dilijev commented Dec 20, 2016

@Cellule any update on this issue?

@Cellule
Copy link
Contributor Author

Cellule commented Dec 20, 2016

It's still something I want to do, but I haven't had the time to finish the implementation in the private repo and it is low priority right now

@dilijev
Copy link
Contributor

dilijev commented Mar 1, 2017

Hi @Cellule, I just realized that this wasn't directly targeted to fix #269 -- I assumed it was related. Is that true?

@Cellule
Copy link
Contributor Author

Cellule commented Mar 2, 2017

@dilijev Well it is related, it doesn't fix it in the sense it doesn't change WScript.LoadScriptFile, but it adds enough functionality to achieve the desired result, ie: load a script file with relative path by using current script's path

@Cellule
Copy link
Contributor Author

Cellule commented Mar 29, 2017

Closing this for now, there is no point really to keep it open until I find the time to fix what is left.

@Cellule Cellule closed this Mar 29, 2017
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

Successfully merging this pull request may close these issues.

6 participants