WebReflection / basicHTML

A NodeJS based, standard oriented, HTML implementation.
ISC License
126 stars 10 forks source link

Improvement suggestion: make sizzle default selector engine? #14

Closed dsadhanala closed 6 years ago

dsadhanala commented 6 years ago

@WebReflection: Thanks for the update 0.13.1 with init() at least now I don't need to patch window, document and CustomElements. however, it's still not great user experience in my opinion and below are few suggestions, may TIOLI.

this is what I still need to do with current implementation.

const { init, HTMLElement, Element }  = require('basichtml');

init();
global.HTMLElement = HTMLElement;

// override selector with sizzle engine
const Sizzle = require('sizzle');
Element.prototype.querySelectorAll = function (css) {
  return Sizzle(css, this);
};

I have forked and made a change here is the commit, let me know your thoughts, I can create pull request if you're ok with this change. I know you want this to be extensible with any selector engine but making this simplified to use will be a big win.

WebReflection commented 6 years ago

I'm sure most devs use webpack, where dynamic import/require is not allowed so can't use the option you have provided. Not sure if rollup compiler allows dynamic require though.

mind if I ask what the hell are you building ? :smile:

basicHTML has few valid use cases I'm keeping in mind but it looks like I don't understand your one. It would help to know more.

since we know query selector from basicHTML doesn't cover most of the CSS selectors, why not make sizzle as default engine and also allow users to override if needed.

because if you use basicHTML for what it was born, creating pageges via viperHTML, nativeHTML, or Service Workers, you realize a fully capable selector engine is 99% useless overhead (so I am back to my previous question)

It would be useful to add HTMLElement also to the global by default, since major focus of basicHTML is to support custom elements, what you think?

Yeah, that makes perfect sense.

this is what I still need to do with current implementation.

... oh no, you shouldn't need to do that ... I have an evil though:

what if instead of name, or as alternative of name, I use module?

require('basichtml').init({
  selector: {
    module() {
      return require('sizzle');
    },
    $(Sizzle, element, css) {
      return Sizzle(css, element);
    }
  }
});
dsadhanala commented 6 years ago

mind if I ask what the hell are you building ?

ha haha top secret project :), just kidding, I'm building a UI library with customElements, so I need to provide simplified build tools also to my consumer teams. I have a inherited config setup for build, test, lint tasks. so all my packages uses webpack build for both production bundling and unit test bundling, this allows me to run tests also in watch mode. I'm sure if I switch to basicHTML then I might file lot of bugs/enhancement request, I hope you don't hate me for filing issues :)

what if instead of name, or as alternative of name, I use module?

My whole point of moving sizzle part of basicHTML is to hide the implementation detail and allow consumer's of this module use with ease, since it's optional we don't import unless they need. I have tried another alternative earlier, where we can have enhanceSelectorEngine take in sizzle something like below, but didn't make sense to me.

Below was my another option to move sizzle out of basicHTML

const basichtml = require('basichtml');
basichtml.init();
basichtml.enhanceSelectorEngine(require('sizzle'));
WebReflection commented 6 years ago

I'm sure if I switch to basicHTML then I might file lot of bugs/enhancement request

hopefully not a lot of them, this project works pretty well even if not so popular

I hope you don't hate me for filing issues :)

issues welcome

My whole point of moving sizzle part of basicHTML

sizzle is heavy unnecessary overhead for most basicHTML uses cases, it makes no sense to ship it included to me, I never needed it so far.

since it's optional we don't import unless they need

it has to ship together (as dependency or in other ways) so optional is unfortunately not how it's going to be.

basichtml.enhanceSelectorEngine(require('sizzle'));

Unfortunately that's not enough because I don't want to couple basicHTML to Sizzle API.

Honestly though, what is missing at this point from the current easy init(...) ?

require('basichtml').init({
  selector: {
    module: () => require('sizzle'),
    $: (engine, element, css) => engine(css, element)
  }
});
dsadhanala commented 6 years ago

I understand, you don't want to include sizzle as dependency, in this case, I think removing dynamic require out should be good enough for webpack bundling use case.

Thank you for being flexible and discussing about alternatives!