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

pathutils_nodejs_support #14512

Closed
wants to merge 1 commit into from

Conversation

bung87
Copy link
Collaborator

@bung87 bung87 commented May 31, 2020

compiled passed on my machine, need review not sure am I doing right.

addNormalizePath need fix not work with jsgen.

@Araq
Copy link
Member

Araq commented May 31, 2020

compiler/pathutils is not a standard lib. is it so useful that it should become one?

@bung87
Copy link
Collaborator Author

bung87 commented May 31, 2020

compiler/pathutils is not a standard lib. is it so useful that it should become one?

I am trying compile nimpretty to js backend, it depends on this module. dont know if it is possible.
I just thought it just string manipulation.

when defined(nodejs):
fs.createReadStream(source.cstring).pipe(fs.createWriteStream(dest.cstring));
else:
os.copyFile(source.string, dest.string)
Copy link
Member

@timotheecour timotheecour May 31, 2020

Choose a reason for hiding this comment

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

but this is the wrong place to branch; instead make os.copyFile work with with nodejs
likewise with other procs.

That way is much more useful as it'll make clients of os work with nodejs.

@timotheecour
Copy link
Member

timotheecour commented May 31, 2020

I am trying compile nimpretty to js backend

I guess that's a fair use case (ideally nim compiler should work on all reasonable backends, including nodejs), but you'll probably face other issues than pathutils; in any case the branching (c vs nodejs) should be done at the right place, ideally at the lowest level place possible (eg the importc proc) unless there's a performance consideration (eg if nodejs has a builtin way to do something that's more efficient than calling the nim way).

in this case, it's probably os.nim for most of those.

right now getCurrentDir is the only place in os.nim with branching for nodejs, but you can add more there and that'll be a generally applicable contribution even if nimpretty ends up not working on nodejs

@bung87
Copy link
Collaborator Author

bung87 commented May 31, 2020

I need listen thoughts, make sure this can be continued, as you mentioned some proc can be place to os.nim, I'll try that so , as far I only have "compile nimpretty to js " goal, maybe it will becoming huge for wrap nodejs libs, when it comes I may create separate nodejs modules wrapper like document listed "modules for js backend"

@timotheecour
Copy link
Member

timotheecour commented May 31, 2020

I'll try that so

yay. anything is the spirit of #14095 is highly appreciated. Instead of creating "modules for js backend", it's best to make existing module work (or work better) for js backend (unless something is js-backend specific of course, like jsconsole)

document listed "modules for js backend"

see tests/js/tstdlib_imports.nim which attempts to list all modules that at least compile with js backend. Of course compile and being fully compatible is a different story.

@bung87
Copy link
Collaborator Author

bung87 commented May 31, 2020

@timotheecour Thanks for your reply! I guess I have new field that I can create PR for :P

@dom96
Copy link
Contributor

dom96 commented Jun 12, 2020

My goodness, stop adding nodejs support to Nim. If you want it then create a Nim package.

@dom96 dom96 closed this Jun 12, 2020
@timotheecour
Copy link
Member

A nim package can't work for something like that, it requires some support logic in nim.

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.

4 participants