Add jsdomparser #13920

juancarlospaco · 2020-04-08T01:41:54Z

Add jsdomparser the DOM Parser for JavaScript target.

alaviss · 2020-04-08T02:36:07Z

Can't this be added directly to dom?

juancarlospaco · 2020-04-08T03:02:16Z

Only Document is used from dom, I do not want to Kitchen Sink the dom module.

timotheecour · 2020-04-08T03:09:58Z

this belongs in dom. I'd agree for a separate module if jsdomparser were a large module or could grow large, but this is not the case.

timotheecour · 2020-04-09T01:07:42Z

lib/js/dom.nim

+  func newDOMParser*(): DOMParser {.importcpp: "(new DOMParser())".}
+    ## DOM Parser constructor.
+
+  func parseFromString*(this: DOMParser; str: cstring; mimeType = "text/html".cstring): Document {.importcpp.}


same comment as in your jsfetch PR, use an enum since there is a finite set of acceptable values for mimeType, see https://developer.mozilla.org/en-US/docs/Web/API/DOMParser. This makes is impossible to misuse that argument, is self documenting, etc... Yes, this requires a helper importcpp proc parseFromStringImpl

don't provide default arg for mimeType since that's what's done in https://developer.mozilla.org/en-US/docs/Web/API/DOMParser (This method has 2 parameters (both required):)

or we can just remove the default mimeType and keep it barebones. JS is a wild place and things can change really fast. We should just provide a barebones wrapper and call it a day.

metagn · 2020-04-09T13:00:12Z

lib/js/dom.nim

@@ -985,6 +985,16 @@ type
    once*: bool
    passive*: bool

+since (1, 3):
+  type DOMParser* = ref object  ## \


Bikeshedding: Should this be DomParser (and newDomParser)? Quoting NEP-1:

In the age of HTTP, HTML, FTP, TCP, IP, UTF, WWW it is foolish to pretend these are somewhat special words requiring all uppercase. Instead treat them as what they are: Real words. So it's parseUrl rather than parseURL, checkHttpHeader instead of checkHTTPHeader etc.

Don't know if DOM counts on the same level as HTTP and HTML but I don't really know if the "level" matters either.

Yeah, it's DomParser and newDomParser indeed.

dom96

Looks good, but see my and @hlaaftana's comment.

dom96 · 2020-04-09T13:17:50Z

lib/js/dom.nim

@@ -9,7 +9,7 @@

 ## Declaration of the Document Object Model for the `JavaScript backend
 ## <backends.html#backends-the-javascript-target>`_.
-
+include "system/inclrtl"


Why is this being added?

Its not needed and should be removed

I know it includes since but you shouldn't have to manually include it.

you need to otherwise you get CT error; but if we merged #11865 we could import it instead of including it

Once again putting it out there that I consider this since thing to be a waste of time.

timotheecour · 2020-04-09T13:55:59Z

lib/js/dom.nim

+
+
+since (1, 3):
+  func newDOMParser*(): DOMParser {.importcpp: "(new DOMParser())".}


this doesn't work, you must've pasted from a webpage and introduced an invisible symbol (see cat -e). you should make sure examples work

also, you should remove outer parens

This should be .importjs: "new DOMParser()"

#13891 (comment) (Doc Test exit non-zero)

timotheecour · 2020-04-09T13:57:58Z

lib/js/dom.nim

+  func newDOMParser*(): DOMParser {.importcpp: "(new DOMParser())".}
+    ## DOM Parser constructor.
+
+  func parseFromString*(this: DOMParser; str: cstring; mimeType: cstring): Document {.importcpp.}


still meh on that; at least provide an enum to make life easier for users eg:

type MimeTypeDom = enum textHtml = "text/html" etc

Please don't. JS grows relatively fast and change really quickly, so we should not spent too much time chasing these things. Different browsers might support more MIME types, and it's not really our place to dictate how to use an API.

I agree with alaviss on this point.

fine, we can later introduce

type MimeTypes = enum textHtml = "text/html".cstring # etc

and use it as:
let a = prsr.parseFromString("<html>...</html>".cstring, $textHtml)

(but not sure what's the difference with fetch API case https://github.com/nim-lang/Nim/pull/12531/files#r403861811

So what, please use enums.
)

dom96 · 2020-04-13T13:06:43Z

lib/js/dom.nim

@@ -9,7 +9,7 @@

 ## Declaration of the Document Object Model for the `JavaScript backend
 ## <backends.html#backends-the-javascript-target>`_.
-
+include "system/inclrtl"


Once again putting it out there that I consider this since thing to be a waste of time.

juancarlospaco · 2020-04-13T13:40:27Z

Without since the already shipped --useVersion: feature wont work, sorry nothing I can do.

Thanks. 🙂

timotheecour · 2020-04-14T03:20:42Z

Once again putting it out there that I consider this since thing to be a waste of time.

/cc @dom96
I kind of agree (although I would call it adding un-needed complexity) but we need some kind of policy on this, we can't have it in some PR's and not on others depending on who's reviewing/merging; this should be debated/resolved in a single location (ie, an RFC); so instead of writing it here, create an RFC

Araq · 2020-04-14T07:08:12Z

And what would be the content of the RFC? "I don't understand the -std=c99 switch"? Opinions are valuable if you shaped them after you understood the problem... Bringing it up again and again wastes my time.

juancarlospaco added 3 commits April 7, 2020 22:36

Add jsdomparser

48f471e

Add jsdomparser

87f8738

Add jsdomparser

6f89c3d

juancarlospaco added 3 commits April 8, 2020 18:17

https://github.com/nim-lang/Nim/pull/13920#issuecomment-610727142

d381256

https://github.com/nim-lang/Nim/pull/13920#issuecomment-610727142

ea65e27

https://github.com/nim-lang/Nim/pull/13920#issuecomment-610727142

bb04479

timotheecour reviewed Apr 9, 2020

View reviewed changes

juancarlospaco added 2 commits April 9, 2020 07:21

https://github.com/nim-lang/Nim/pull/13920#discussion_r405932909

4f3e290

Merge branch 'devel' of https://github.com/nim-lang/Nim into jsdomparser

6172d73

metagn reviewed Apr 9, 2020

View reviewed changes

dom96 approved these changes Apr 9, 2020

View reviewed changes

timotheecour reviewed Apr 9, 2020

View reviewed changes

juancarlospaco added 2 commits April 10, 2020 05:32

Merge branch 'devel' of https://github.com/nim-lang/Nim into jsdomparser

92f5394

https://github.com/nim-lang/Nim/pull/13920#discussion_r406502592

2c2739e

juancarlospaco requested a review from Araq April 11, 2020 04:41

dom96 approved these changes Apr 13, 2020

View reviewed changes

dom96 merged commit 0a84219 into nim-lang:devel Apr 13, 2020

juancarlospaco deleted the jsdomparser branch April 13, 2020 13:41

timotheecour mentioned this pull request Apr 14, 2020

misc tracking of unresolved issues/comments timotheecour/Nim#87

Open

timotheecour mentioned this pull request Apr 14, 2020

fix newDomParser #13981

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jsdomparser #13920

Add jsdomparser #13920

juancarlospaco commented Apr 8, 2020

alaviss commented Apr 8, 2020

juancarlospaco commented Apr 8, 2020

timotheecour commented Apr 8, 2020

timotheecour Apr 9, 2020 •

edited

Loading

alaviss Apr 9, 2020

metagn Apr 9, 2020

Araq Apr 9, 2020

dom96 left a comment

dom96 Apr 9, 2020

Clyybber Apr 9, 2020

timotheecour Apr 9, 2020

juancarlospaco Apr 9, 2020

Clyybber Apr 9, 2020

timotheecour Apr 9, 2020

dom96 Apr 13, 2020

timotheecour Apr 9, 2020 •

edited

Loading

Araq Apr 9, 2020

juancarlospaco Apr 10, 2020 •

edited

Loading

timotheecour Apr 9, 2020 •

edited

Loading

alaviss Apr 9, 2020

Araq Apr 9, 2020

timotheecour Apr 10, 2020

dom96 Apr 13, 2020

juancarlospaco commented Apr 13, 2020

timotheecour commented Apr 14, 2020 •

edited

Loading

Araq commented Apr 14, 2020



		since (1, 3):
		func newDOMParser*(): DOMParser {.importcpp: "(new DOMParser())".}

Add jsdomparser #13920

Add jsdomparser #13920

Conversation

juancarlospaco commented Apr 8, 2020

alaviss commented Apr 8, 2020

juancarlospaco commented Apr 8, 2020

timotheecour commented Apr 8, 2020

timotheecour Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dom96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juancarlospaco Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

timotheecour Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juancarlospaco commented Apr 13, 2020

timotheecour commented Apr 14, 2020 • edited Loading

Araq commented Apr 14, 2020

timotheecour Apr 9, 2020 •

edited

Loading

timotheecour Apr 9, 2020 •

edited

Loading

juancarlospaco Apr 10, 2020 •

edited

Loading

timotheecour Apr 9, 2020 •

edited

Loading

timotheecour commented Apr 14, 2020 •

edited

Loading