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 Element Mixin #86

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add Element Mixin #86

wants to merge 3 commits into from

Conversation

bedeoverend
Copy link
Contributor

@bedeoverend bedeoverend commented Dec 6, 2017

Addresses #79

Adds the new ES6 SimplaElement mixin, converted from the ES5 Polymer Behavior at https://github.com/simplaelements/simpla-element-behavior to be a generic mixin for v1 Web Components.

Usage

import SimplaElement from 'simpla/mixins/element';

class SimplaImg extends SimplaElement(HTMLElement) {
    static get simplaConfig() {
      return {
        type: 'Image',
        dataProperties: [ 'src', 'alt' ],
        events: [ 'change' ]
      };
    }
}

Major Changes

  • No longer requires Polymer
  • Now reads in static simplaConfig property rather than a factory function
  • No longer supports "watching" data properties. Now events which should trigger a set back to Simpla need to be declared in the config
  • Now requires user to implement the updateFromSimpla and updateSimplaBuffer methods rather than declaring setCallback / getCallback methods in the config, if they want to override the set / get behaviour e.g.
class SimplaImg extends SimplaElement(HTMLElement) {
    static get simplaConfig() {
      return {
        type: 'Image',
        dataProperties: [ 'src', 'alt' ],
        events: [ 'change' ]
      };
    }

    updateSimplaBuffer() {
      if (this.src === DEFAULT_SRC) {
        return null;
      }

      return super.updateSimplaBuffer();
    }
}

May need to change the public names it adds - current choices were just from the private names implemented in the old behavior

Also tweaks how readonly / editable interact with one another, essentally the same behaviour I believe, but a little bit more robust

@bedeoverend
Copy link
Contributor Author

Also should add tests to this branch before this is included in a release, just in the process of converting the old tests, but that can come after this is merged in a new PR. Currently having some issues with using Skate's Jest environment package to enable using customElements in Node.

@bedeoverend
Copy link
Contributor Author

bedeoverend commented Dec 6, 2017

Oh other note - should we just have all mixins accessible at simpla/mixins? e.g. import { Element } from 'simpla/mixins'

EDIT: Actually probably not, because we're compiling as a UMD (accessible at window.SimplaElement), so it's useful for people to be able to import individual mixins if they're not tree shaking.

@bedeoverend bedeoverend mentioned this pull request Dec 6, 2017
12 tasks
@madeleineostoja
Copy link

To the last point: yes, once they (and elements) are JS. Then there won't be any reason for them to be UMDs either. But for now yeah separate files and used as globals is the only way.

@madeleineostoja
Copy link

madeleineostoja commented Dec 6, 2017

Wait sorry just saw this is a JS file now - it should be an HTML import (for deduping), and consumed as a global, so that components themselves don't require build steps just to function.

Again, once everything is JS then bundlers / ES modules take care of it. Until then it's not worth requiring a bundler buildstep on components just for this.

@madeleineostoja
Copy link

And yes, we should definitely change the names of those hooks. I still vote for just set and get, or add a suffix if we want to be more verbose

@bedeoverend
Copy link
Contributor Author

bedeoverend commented Dec 8, 2017

@seaneking would you be able to setup the build for that? Been awhile since I've setup a gulpfile, particularly not with the new version. We'll need that to pipe it into HTML I believe. Unless we just use rollup and literally concat inside a <script>...</script> tag - probably could do that? Means wouldn't need to add in gulp which'd be nice

@madeleineostoja
Copy link

Yeah don't see why we'd need gulp just to put it in a HTML file, just bundle it and manually write to a HTML file

Copy link

@madeleineostoja madeleineostoja left a comment

Choose a reason for hiding this comment

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

Components are going to be JS now, so this can just export an ES6 module as-is. Still want to change the names of those callbacks though

@madeleineostoja
Copy link

Another thing to consider - if we ship UMD definitions of elements (for simpla-component-loader idea we talked about) as well as ES6 exports, should this still be a global (window.Simpla.Element or something) so that code isn't duped? would really rather not need a buildstep, but then again Simpla itself doesn't ship a proper ES6 module. Maybe this could be part of #74, make a umd dir for current simpla.min.js as well as UMDified adapters, mixins, etc, and make the stuff in the root (and mixins, adapters dirs) proper module exports for bundlers.

@bedeoverend
Copy link
Contributor Author

So not quite sure I fully understand, as deduping won't be affected by putting into global namespace. But

make a umd dir for current simpla.min.js as well as UMDified adapters, mixins, etc, and make the stuff in the root (and mixins, adapters dirs) proper module exports for bundlers.

I think we should do this. Standard pattern for ES / UMD exports for all entry points we have. For UMDs that also means following a standard naming convention. Will make a note on #74 as more relevant there than here.

@madeleineostoja
Copy link

deduping won't be affected by putting into global namespace

Wires crossed, I didn't mean deduping resources in the network sense, I meant that you wouldn't have to pull and rebundle the mixin for every compiled UMD element definition.

UMDs as a general pattern for Simpla core sounds good though 👍 Right now the dist files we have are in a bit of a limbo between the two

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