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 jsheaders #56

Merged
merged 14 commits into from
Dec 26, 2020
Merged

Add jsheaders #56

merged 14 commits into from
Dec 26, 2020

Conversation

juancarlospaco
Copy link
Contributor

@juancarlospaco juancarlospaco commented Dec 14, 2020

This was referenced Dec 14, 2020
@juancarlospaco juancarlospaco mentioned this pull request Dec 15, 2020
when not defined(js) and not defined(nimdoc):
{.fatal: "Module jsheaders is designed to be used with the JavaScript backend.".}

type Headers* = ref object ## HTTP Headers for the JavaScript target.
Copy link
Member

Choose a reason for hiding this comment

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

  • I think this should be ref object of JsRoot
  • we also need to fix similar errors in stdlib:
lib/js/dom.nim:1213:28:  TouchList* {.importc.} = ref object of RootObj
lib/js/dom.nim:1293:26:  TimeOut* {.importc.} = ref object of RootObj
lib/js/jsconsole.nim:16:17:type Console* = ref object of RootObj
lib/js/jsffi.nim:87:15:  JsObject* = ref object of JsRoot
lib/js/jsffi.nim:89:27:  JsAssoc*[K: JsKey, V] = ref object of JsRoot

PR's welcome!

Copy link
Contributor Author

@juancarlospaco juancarlospaco Dec 19, 2020

Choose a reason for hiding this comment

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

I was thinking it was correct because stdlib uses it, maybe documentation needs to be more explicit on that. 🤔

Too bad theres not a Linter for that.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

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

LGTM after addressing last 2 points

@juancarlospaco juancarlospaco mentioned this pull request Dec 25, 2020
@timotheecour
Copy link
Member

timotheecour commented Dec 26, 2020

@juancarlospaco ok, can you find another reviewer to get a 2nd LGTM so we can merge this?

@timotheecour timotheecour merged commit 23be8e3 into nim-lang:master Dec 26, 2020
@juancarlospaco juancarlospaco deleted the js1 branch December 26, 2020 22:52
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.

3 participants