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

Support GHC JS backend #135

Merged
merged 54 commits into from
Apr 7, 2024
Merged

Support GHC JS backend #135

merged 54 commits into from
Apr 7, 2024

Conversation

hamishmack
Copy link
Member

No description provided.

@JoshMeredith
Copy link
Contributor

Should we remove callbacks? That module has been upstreamed into base.

@hamishmack hamishmack changed the title Fix build with JS backend Support GHC JS backend Feb 5, 2024
Copy link
Contributor

@doyougnu doyougnu left a comment

Choose a reason for hiding this comment

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

Well done Josh and Hamish! I've tried to review best I could, but it could probably benefit from a more thorough review from someone who's worked on ghcjs. IMO it LGTM, just some minor fixes.

@@ -1064,15 +1063,6 @@ realFloatToJSON d
| otherwise = doubleValue (realToFrac d)
{-# INLINE realFloatToJSON #-}

scientificToNumber :: Scientific -> Number
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember this too well. I see that it was used in an instance that's commented out, and it's not exported - so I'd guess that I removed it because of an unused function warning.

JavaScript/JSON/Types/Internal.hs Show resolved Hide resolved
JavaScript/Web/MessageEvent.hs Outdated Show resolved Hide resolved
@@ -2,7 +2,8 @@
ForeignFunctionInterface, JavaScriptFFI, EmptyDataDecls,
TypeFamilies, DataKinds, ScopedTypeVariables,
FlexibleContexts, FlexibleInstances, TypeSynonymInstances,
LambdaCase, MultiParamTypeClasses, DeriveGeneric #-}
LambdaCase, MultiParamTypeClasses, DeriveGeneric,
TypeOperators #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

TypeOperators are not used in the diff, is this a GHC API induced change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hazy on this too, but IIRC there was a warning for it. It might have been allowed in earlier versions of GHC without warning.

ghcjs-base.cabal Outdated
@@ -1,6 +1,6 @@
cabal-version: 3.0
name: ghcjs-base
version: 0.2.1.0
version: 1.0.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning for a 1.0 release? Are we promising something stable? I would think we should bump a major version, for sure, but a 1.0 seems is too much, especially because I envision we'll have to make our own ghcjs-internal and ghcjs-base split.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hamishmack suggested this on slack a while ago as a way to break version bounds with projects using GHCJS. IMO, there's also the possibility of releasing with a new name, but I don't think it's necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the problem with a new name is that they should be mutually exclusive. If we keep the same name there is no way anyone will wind up with both in their build plan.

I wonder if we should use 0.8 or something to avoid confusion. It seems unlikely we will want to release any more non arch(javascript) versions.

Comment on lines +849 to +851
if (i >= 0) {
v += x.u1[i].toString(16);
i--;
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels like it could be inlined into the previous while loop, probably not a worthwhile optimization without profiling though. Just wanted to mention it.

Comment on lines +853 to +854
for (; i >= 0; i--) {
v += x.u1[i].toString(16).padStart(4, '0');
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, this screams for some loop fusion, but best left for another time.

jsbits/jsstring.js Show resolved Hide resolved
}
}
return s + String.fromCharCode.apply(this, a);
return new TextDecoder().decode(arr.u8.subarray(off, off+len));
Copy link
Contributor

Choose a reason for hiding this comment

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

chef's kiss

test/Tests/Properties.hs Outdated Show resolved Hide resolved
@luite
Copy link
Member

luite commented Feb 12, 2024

I had a quick read already, but will do a proper review at the end of today

@hamishmack hamishmack merged commit a8227ff into master Apr 7, 2024
1 check failed
sevanspowell added a commit to sambnt/ghcjs-base that referenced this pull request May 3, 2024
- Fixed some issues with the JS backend syntax translation introduced by
  ghcjs#135.
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