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

Add jsre #14870

Merged
merged 12 commits into from Jul 3, 2020
Merged

Add jsre #14870

merged 12 commits into from Jul 3, 2020

Conversation

juancarlospaco
Copy link
Collaborator

lib/js/jsre.nim Outdated Show resolved Hide resolved
lib/js/jsre.nim Outdated Show resolved Hide resolved
lib/js/jsre.nim Outdated Show resolved Hide resolved
lib/js/jsre.nim Outdated Show resolved Hide resolved
lib/js/jsre.nim Outdated Show resolved Hide resolved
lib/js/jsre.nim Outdated Show resolved Hide resolved
lib/js/jsre.nim Outdated Show resolved Hide resolved
lib/js/jsre.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member

timotheecour commented Jul 1, 2020

to fix test failure, you can update kochdocs as follows (untested but shouldn't be hard to make it work):

# add this:
proc isJsOnly(file: string): bool = file.isRelativeTo("lib/js")

# update `commands[i] = nim & " doc $# --git.url:$# --outdir:$# --index:on $#" % [nimArgs2, gitUrl, destPath, d]`
# to:

let isJsOnly = 
var extra = ""
if isJsOnly(d): extra.add " --backend:js"
commands[i] = nim & " doc $# $# --git.url:$# --outdir:$# --index:on $#" % [extra, nimArgs2, gitUrl, destPath, d]

(let me know if anything is unclear)

lib/js/jsre.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member

timotheecour commented Jul 1, 2020

@juancarlospaco this test will fail:

doAssert jsregex.exec(r"nim javascript") == @["nim"]

=>

doAssert jsregex.exec(r"nim javascript") == @["nim".cstring]

(as you can see locally with nim doc --backend:js -r lib/js/jsre.nim)

@timotheecour
Copy link
Member

timotheecour commented Jul 1, 2020

and you still need to update kochdocs as described in #14870 (comment)

otherwise CI will fail with:

2020-07-01T20:56:15.7235670Z /Users/runner/runners/2.263.0/work/Nim/Nim/lib/js/jsre.nim(6, 13) Error: importjs pragma requires the JavaScript target

after that LGTM for me

@timotheecour
Copy link
Member

timotheecour commented Jul 1, 2020

  • in a cleanup PR (if you don't want to keep editing this PR and finally ship it), it would be nice to cross reference this from std/re and std/nre:

eg:

# in re.nim
when defined(js):
  {.error: "This library needs to be compiled with a c-like backend, and depends on PCRE.".}

=>

when defined(js):
  {.error: "This library needs to be compiled with a c-like backend, and depends on PCRE; see std/jsre for js backend".}
  • in future work (but obviously much more involved), one could contemplate lifting that restriction and making std/{re,nre} work with both c and js backend, using std/jsre as implementation for the re/nre API when using js mode (unless pkg/regex is faster than std/jsre which I kind of doubt).

I hope it works :|

running locally ./koch --localdocs:htmldocs2 docs is your friend, for faster turnaround ;-)

@Araq
Copy link
Member

Araq commented Jul 2, 2020

Shouldn't this be a "fusion" module?

@timotheecour
Copy link
Member

timotheecour commented Jul 2, 2020

if regex is stdlib territory for c backend, it makes sense to have regex for js in stdlib as well; it should reduce friction when porting working nim code from c to js; and as said above, in future work it would be nice for this to be transparent (so that re/nre can use jsre under the hood for js backend). And this could be gradual (1 proc at a time).

I don't think pkg/regex is "the answer" in the js case since I expect it to have large overhead over jsre but maybe @juancarlospaco can do a quick benchmark? /cc @nitely

@pietroppeter
Copy link
Collaborator

For an example where it would help to have jsre in stdlib, nim-markdown would be able to support js backend.

@Yardanico
Copy link
Collaborator

Yardanico commented Jul 2, 2020

@pietroppeter did you look into https://github.com/nitely/nim-regex? I think it might be a good fit too

@timotheecour
Copy link
Member

timotheecour commented Jul 2, 2020

@Yardanico see remark here #14870 (comment), this at very least needs a performance comparison, assuming https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions would be natively (or at least efficiently) implemented whereas nim-regex would incur large overhead on js backend

@pietroppeter
Copy link
Collaborator

pietroppeter commented Jul 2, 2020

yes, I was aware of nim-regex, having seen it mentioned in #7640 (which could actually be closed if this is merged), but it is good that this is also mentioned in this conversation. I still think this jsre would be a good fit for stdlib:

  • we have it for C backend so it makes sense for JS (@timotheecour's point above)
  • I would expect native JS regex to be better than a nim implementation (although I guess it could also not be the case)
  • also if you use native JS regex you would not need to ship in your js file Nim's implementation
  • regarding markdown library above, it currently has no dependecies other than nim and it would be nicer to keep it that way

@Araq Araq merged commit 4f6acf2 into nim-lang:devel Jul 3, 2020
@juancarlospaco juancarlospaco deleted the jsre branch July 3, 2020 21:36
@timotheecour timotheecour mentioned this pull request Mar 13, 2021
2 tasks
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.

None yet

5 participants