Skip to content
This repository has been archived by the owner on Oct 4, 2020. It is now read-only.

Add missing Element-Attr API functions, add Attr type. #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

manpages
Copy link
Contributor

This commit also adds natively-typed API to work with attributes.

Notice that here we also take a liberty of explicitly marking legacy and
nonsense-fields of Attr IDL as legacy instead of IDL names of those.

Again, a ridiculously stupid, yet somewhat complete test suite is attached to PR.

I don't expect this PR to get merged right away, instead I'd like to hear
opinions regarding what to do with this code and Attr API.

This commit also adds natively-typed API to work with attributes.

Notice that here we also take a liberty of explicitly marking legacy and
nonsense-fields of Attr IDL as ``legacy`` instead of IDL names of those.
@garyb
Copy link
Member

garyb commented Aug 23, 2015

I'm not sure Attr should be a record type synonym, as giving it field accessors this way doesn't encapsulate the fact that value is mutable, and also it suggests you can "make" Attr values by just using a normal record, which isn't the case.

My original thought was to just have foreign import data Attr :: * as with all the the other types, and then implement a bunch of functions for reading localName, name, namespaceURI, prefix of the form Attr -> whatever, value :: forall eff. Eff (dom :: DOM | eff) (Nullable String) and setValue :: blah.

g f (Just v) = toNullable $ Just (f v)

(>>) :: forall a b m. (Monad m) => m a -> m b -> m b
(>>) x y = x >>= \_ -> y
Copy link
Member

Choose a reason for hiding this comment

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

This is defined as (*>) in Control.Applicative - no need for (>>) in PureScript as Applicative is a superclass of Monad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old habits die hard.

@manpages
Copy link
Contributor Author

My original thought was to just have foreign import data Attr :: *

I like this approach and will implement it, test and force-push.

@manpages
Copy link
Contributor Author

I'm confused though, quoting IRC —

| garyb: just to clarify. You propose we interface with createAttribute method in order to make an instance of Attr?
| I'm incompetent in the state of Attrs throughout the history of DOM.
| Also, createAttribute reutrns (used to return) AttributeNode...
| But I agree that my API is absolutely terrible.

@garyb
Copy link
Member

garyb commented Aug 23, 2015

I'm also not totally up on the DOM and previous versions either, so I'm just trying to stick to whatever the IDLs say in the DOM 4 draft really... it looks like there is no way of instantiating an Attr directly in the current draft - Attr values can be obtained through the attributes collection on an Element.

I appreciate the contribution, but since I'm being so picky about this would you rather I just did it instead?

@garyb
Copy link
Member

garyb commented Aug 23, 2015

I wouldn't say your API is terrible either 😉 I just have specific ideas about how this library should be built, to keep it as unopinionated as possible while providing typed access to the DOM API.

That does make it more user unfriendly than normal libraries, but this is supposed to be a starting point to build useful libraries on top of while providing a set of common types that they can share, and prevent the need of resorting to writing code in the FFI for their implementations.

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

Successfully merging this pull request may close these issues.

2 participants