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 a build that allows for ES module use #8

Merged
merged 2 commits into from
Feb 1, 2017
Merged

Add a build that allows for ES module use #8

merged 2 commits into from
Feb 1, 2017

Conversation

matthewp
Copy link
Owner

This adds a build so that custom-attributes can be used as an ES module.

import { customAttributes, CustomAttributeRegistry } from
'custom-attributes';

The above imports each, without anything being set on the window. If
you just want the registry you can do:

import CustomAttributeRegistry from 'custom-attributes/registry';

and create your own instance.

The same script tag will still work which sets the global:

<script src="./node_modules/custom-attributes/attr.js"></script>

Closes #7

cc @bedeoverend

This adds a build so that custom-attributes can be used as an ES module.

```js
import { customAttributes, CustomAttributeRegistry } from
'custom-attributes';
```

The above imports each, without anything being set on the `window`. If
you just want the registry you can do:

```js
import CustomAttributeRegistry from 'custom-attributes/registry';
```

and create your own instance.

The same script tag will still work which sets the global:

```html
<script src="./node_modules/custom-attributes/attr.js"></script>
```

Closes #7
@matthewp matthewp requested a review from bedeoverend January 31, 2017 12:16
// chlidList
else {
forEach.call(m.removedNodes, downgrade);
forEach.call(m.addedNodes, upgrade);
Copy link
Collaborator

Choose a reason for hiding this comment

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

forEach is now undefined - think it was at the top of the file? Also I'm pretty sure mutations passed to the observer callback is already an Array so should only need the forEach.call on the above two lines.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, CI caught this as well, will update.

@bedeoverend
Copy link
Collaborator

Nice! Only thing I find a bit odd is how I can get CustomAttributesRegistry from two spots - personally, I don't think I'd ever use custom-attributes/registry if I can just:

import { CustomAttributesRegistry } from 'custom-attributes'

Is it worth maybe leaving out registry, or not having it in custom-attributes?

index.js Outdated

var customAttributes = new CustomAttributeRegistry(document);

export { customAttributes, CustomAttributeRegistry }
Copy link
Collaborator

Choose a reason for hiding this comment

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

@matthewp what do you think of using:

export default customAttributes
export { CustomAttributeRegistry }

?
Import interface then:

import customAttributes, { CustomAttributeRegistry } from 'custom-attributes';

Personally, like the idea of customAttributes being the default there, with CustomAttributeRegistry as a kind of extra utility.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, agree, will update.

@matthewp
Copy link
Owner Author

matthewp commented Feb 1, 2017

@bedeoverend

Is it worth maybe leaving out registry, or not having it in custom-attributes?

Yeah, the reason I left in custom-attributes/registry was in case you didn't want a default customAttributes for the global document. But if you don't need that I'll just remove the second import.

@bedeoverend
Copy link
Collaborator

@matthewp for me personally, no - though thanks for the consideration! 😄

@bedeoverend
Copy link
Collaborator

Oh apologies - I just re-read that. Right, yeah for my use case I'm looking to not have a global registry added by default. Yeah perhaps it is worth keeping in a separate file by default? Certainly feels cleaner to have it all bundled into one file. Is there much downside to having the default registry setup, even if you're not going to use it?

@matthewp
Copy link
Owner Author

matthewp commented Feb 1, 2017

@bedeoverend I think we can keep custom-attributes/registry if you don't want to have it set up by default but still have it set up in the package's entry page.

I guess the downside would be that MutationObservers are set up.

This restructures the API so that customAttributes is the default.
@matthewp
Copy link
Owner Author

matthewp commented Feb 1, 2017

@bedeoverend Ok, I updated it so that the API now works like:

import customAttributes, { CustomAttributeRegistry } from 'custom-attributes';

Or if you don't want the global document to be set up:

import CustomAttributeRegistry from 'custom-attributes/registry';

Sound good?

@bedeoverend
Copy link
Collaborator

@matthewp LGTM - thanks!

@matthewp matthewp merged commit 6ac53b2 into master Feb 1, 2017
@matthewp matthewp deleted the build branch February 1, 2017 11:58
@matthewp
Copy link
Owner Author

matthewp commented Feb 1, 2017

Released as 1.1.0

@bedeoverend
Copy link
Collaborator

Awesome - thanks @matthewp!

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.

2 participants