Skip to content

Update to psc 0.9.1 #79

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

Merged
merged 3 commits into from
Jun 11, 2016
Merged

Update to psc 0.9.1 #79

merged 3 commits into from
Jun 11, 2016

Conversation

maddie927
Copy link

Also formats code for 80 columns & no vertical aligning

@paf31
Copy link
Contributor

paf31 commented Jun 2, 2016

Looks good to me, thanks! @ethul Any thoughts on this before I merge it?

@ethul
Copy link
Contributor

ethul commented Jun 2, 2016

Looks good to me!

Should we make any changes regarding #76 at this point?

We can maybe do this separately or even keep the img function as-is and add something like

img_ :: Array Props -> ReactElement

Perhaps this is better in a separate PR though. Not sure if it should hold back a release for 0.9.1 compatibility.

@jasonzoladz
Copy link

Agree on separate PR for #76. (I'd like to upgrade to 0.9.1 as soon as practical -- and update the tiny addons library -- but purescript-react is my bread-and-butter right now.)

@paf31
Copy link
Contributor

paf31 commented Jun 5, 2016

We'll probably need to update purescript-react-dom at the same time, but that's blocked by purescript-dom, right?

@maddie927
Copy link
Author

maddie927 commented Jun 5, 2016

Right. purescript-dom is pretty close to being done, but can't quite be finished until purescript-datetime is done. The biggest outstanding thing there is that JSDate is gone.

Overall the new API is much nicer for PS, but I'm no longer sure how to type the APIs in purescript-dom that return JS Date instances. I'd actually really like to convert them to the new DateTime type because it'll make using them much easier. The JSDateInstance -> DateTime function should live in purescript-datetime though.

This might be premature too, since @garyb isn't done yet. He's probably already considered this.

@garyb
Copy link
Member

garyb commented Jun 5, 2016

@spicydonuts this is what I have in mind for JSDate: https://github.com/garyb/purescript-js-date. It's probably pretty much finished as it is, but datetime isn't quite yet, I will be done with both later this evening though.

Since we don't tend to use classes in purescript-dom for anything, I'd probably prefer not do any magic again here either. There are functions in purescript-js-date that should make the conversion pretty easy - but also there's another difficulty with using a class to do it automatically, DateTime doesn't admit invalid dates the way JSDate does, so the function is typed JSDate -> Maybe DateTime.

@maddie927
Copy link
Author

Ah, perfect! Would you ever consider using fromJust in a DOM API wrapper that should never return invalid dates? I'm not sure if there are any such guarantees, just curious.

(and don't forget to change js-date's package name 🙂)

@garyb
Copy link
Member

garyb commented Jun 5, 2016

Yeah definitely, that's a good point. If we know that a date is valid from some source then usin it with a fromJust is totally fine.

(And thanks for alerting me to the bower.json! I probably would have missed it).

@maddie927
Copy link
Author

This is one of the Date APIs: https://developer.mozilla.org/en-US/docs/Web/API/File/lastModifiedDate

Sounds like it's always valid since missing dates become new Date(), unless the file can have a date value outside the range JS Dates support. Maybe DateTime would be a little misleading in this case.. I'd expect that to be Nothing if the file didn't have a modified date. It makes sense once you know how both the DOM and purescript-js-date APIs work..

@paf31 paf31 merged commit 326dbb6 into purescript-contrib:master Jun 11, 2016
@paf31
Copy link
Contributor

paf31 commented Jun 11, 2016

Thanks! I'll work on react-dom now.

@maddie927
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants