Juris-M / citeproc-js-docs

Documentation for citeproc-js
13 stars 14 forks source link

Rewrite dynamic demo with classes #3

Open fbennett opened 8 years ago

fbennett commented 8 years ago

@dsifford Thanks to your framework in citesupport-es6.js, I'm learning how JS classes work (and much the happier for it).

It might save you some time if I go ahead and rewrite the demo using class syntax. Take a look at the code layout -- I've set it up to use a class for the main code, mixed in with hook functions specific to the raw DOM environment, and extended with an init function that is specific to the demo.

The idea is to give you a core logic-only module that you can extend with the hook functions to speak with documents in specific projects. Does it look like this layout will do that?

dsifford commented 8 years ago

Awesome, glad you like the new syntax! Of note, if you think ES6 classes are nice, you should REALLY check out TypeScript... For me, the step from ES5 to ES6 felt the same as the step from ES6 to TypeScript (if we're measuring on the, "holy cow, this is really nice" scale). (Especially if you use atom-typescript).

Ok, phew, ending sales pitch...

Looks good so far. One thing I've never seen before that you're using is this. Is that a legal syntax move? If so, I'm totally jealous on how fast you can go from beginner-to-awesome when learning new syntaxes. Where can I read more about that?

If all you're trying to do is create a class that others can extend (ie "hook into"), all you'd need to do is just create a class normally and export it...

// file1.js
export class MyClass {

  constructor(num) {
    this.num = num;
  }
}

// file2
import { MyClass } from './file1.js';

class AnotherClass extends MyClass {

  constructor(num, str) {
    super(num); // <-- important
    this.str = str;
  }
}

So, in a nutshell, all of the methods and properties from MyClass are available on AnotherClass plus whatever the user wants to add in.

The only other tiny thing I'll mention (which is mostly just a preference): Ditch the use of var. If you need a variable that re-assignable, use let. If you need a variable that isn't, use const.

One common misconception that new ES6 users have is that const behaves like an immutable variable. That's false. You just can't reassign it. So both of these are legal...


const arr = [];
arr.push(1);
arr.push(2);
console.log(arr);
// [1, 2]

arr = 2;
// illegal

const obj = {
  foo: 'bar',
};
obj['bar'] = 'baz';
console.log(obj);
// Object {foo: "bar", bar: "baz"}

obj = [1,2];
// illegal

Hopefully that helps! Let me know if there's anything else I can help with. Awesome work so far!

fbennett commented 8 years ago

I'm still just stumbling around, but the construct came from the Mozilla MDN docs. I'm thinking that the mixin is needed in the demo, at least, because I want to extend the base class twice -- once with the DOM methods, and again with the extra stuff that sets up the spoofed document environment on page load. I guess maybe I could just extend the class twice, though.

dsifford commented 8 years ago

Ohh, okay yeah maybe I have seen that before.

Here's a pretty popular article on the topic of mixins which you might find helpful (the focus is on React, but I think the message still carries and you might be able to extract some good stuff from it).

I'll check back tomorrow to see the good stuff you come up with. Bed time!

fbennett commented 8 years ago

Okay. I don't actually have anything that will require super access pinned to the parent state, so I guess I can just nest the two extensions and let prototyping sort things out. In any case, once the base class is done, folks can put things together with favored syntax locally.

I'll see if I can get sething running soon. On May 12, 2016 1:15 PM, "Derek Sifford" notifications@github.com wrote:

Ohh, okay yeah maybe I have seen that before.

Here's a pretty popular article https://medium.com/@dan_abramov/mixins-are-dead-long-live-higher-order-components-94a0d2f9e750#.5zf1d7zck on the topic of mixins which you might find helpful (the focus is on React, but I think the message still carries).

I'll check back tomorrow to see the good stuff you come up with. Bed time!

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/Juris-M/citeproc-js-docs/issues/3#issuecomment-218656278

fbennett commented 8 years ago

Will be tomorrow before it runs, but the class-based reimplementation is coming along. Should be much easier to follow what's going on in there.

fbennett commented 8 years ago

@dsifford Now working on the handler for edit operations, pulling everything under one roof to make it easier to follow---and thinking forward to deployments, I have a couple of questions.

Users might change the document in various ways between citation edits, either by (1) removing cited text, (2) by cut-and-paste rearrangement of cited text, (3) by cut-and-paste replication of cited text, or (4) by pasting text in from another document.

The first three are easy to account for, since we have citationID on the citation nodes and in citationByIndex records. In word processor context (LibreOffice, Word), ref manager plugins have to handle (4) as well, and I'm wondering what the situation will be in tinyMCE. When you paste text into tinyMCE, does it come in with its HTML formatting, or can it be stripped to plain text? If the latter, we can ignore (4).

Another related question is how you'll be storing citation data in the document. If we have to handle (4) -- that is, if the citations come in when text is pasted from another document written with citesupport -- then citations will need to carry a copy of their own data. Is that feasible, perhaps using something like a data URL?

The issues around (4) are really thorny, so in the short term it would be much simpler to eliminate that use case by stripping HTML (or at least citation nodes) from pasted text. (It could be addressed later.)

What do you think?

dsifford commented 8 years ago

Hey, sorry for the delayed reply.

I'll start off by saying that TinyMCE is a huuuuuuuuuuge pain in the butt. I wouldn't have picked it if I had the option just because of how bloated and aged its API is (I'm more of a Draft.js guy).

That said, yes, HTML formatting comes along for the ride. I really don't know how (or if) you'd be able to move citation information from a blog post to another editor using TinyMCE.

My first thought was that, since HTML formatting comes along, you might be able to attach html data attributes to the surrounding tag that carry the required info as stringified JSON. Buuuuut, I just tried that, and it doesn't work. It looks like TinyMCE strips everything but the basic formatting tags (strong, em, etc).

can it be stripped to plain text?

Yes, but not many people know about it. You can either right click and select "paste as plain text" or hit ctrl+shift+V.

how you'll be storing citation data in the document.

Same as above -- serialized json stored in a data-attribute so it's always bound to the document.

I really have no idea how to tackle this. If someone wanted to copy and paste a cited paragraph from another blog (without accessing that blog's editor), I don't think it'll be possible. If they have access to the editor, then they can just switch to the raw text mode and copy the entire source (data-attributes and all).

it would be much simpler to eliminate that use case by stripping HTML (or at least citation nodes) from pasted text

Welp, at the very least, we're covered there!

Hopefully that helps clarify your questions. Let me know if there's anything else I can clarify.

(Sorry if my response is confusing: Typing while groggy! Long day...)

fbennett commented 8 years ago

That's all very clear---and good news, actually. If citations themselves can't be copied into tinyMCE, that simplifies things a lot, so it's good to hear.

Higher math can come later---and by then you may be looking at another editor anyway.

fbennett commented 8 years ago

Okay! The es6 version is working now, and I've merged it to master.

There are three classes in the object. I'm not sure how the code should be broken out, so I've left the works in a single file. If a different layout will make it easier to work with, feel free to rearrange things and file a PR.

I've tried to bring it within reach of serviceability for production. There are some notes at the top of the file on things that will need tweaking.

Looking forward to hearing how you get on with it. If there are problems, just give a shout!

dsifford commented 8 years ago

@fbennett Sorry this has taken me so long to get to! About to check it out now. I'll report back after I get a chance to soak it all in. 👍

fbennett commented 8 years ago

Take your time! The day job comes first.

I made a few changes yesterday, but it's stable now. The page was broken on iOS. I don't have good access to Mac systems, but poking around it looked like Safari 9 didn't yet support const and let, so I reverted to var everywhere (sorry about that). Classes are supported in iOS, but my subclassing syntax was apparently not compatible, so I just banged everything into one class (with safeStorage as a separate chunk). Works in iOS now.

I'll align master and draft. Feel free to make any changes you want on draft, or copy to a separate project. Any questions, just give a shout.

dsifford commented 8 years ago

@fbennett Ah, yeah I suppose I should have mentioned that you'd have to run the ES6 stuff through babel for it to work everywhere.

I can set up a babel workflow for you if you wanted to keep the code 100% future-proof. Let me know.

fbennett commented 8 years ago

Never say no to free stuff! What's babel?

fbennett commented 8 years ago

Ah, got it. Quick question - does it transform unicode regexp?

dsifford commented 8 years ago

Wasn't sure, but according to stack-overflow, I think so!

http://stackoverflow.com/questions/280712/javascript-unicode-regexes

dsifford commented 8 years ago

I made a branch and I'm slowly making formatting changes (aligned with eslint airbnb) now. I'll add a babel runner to this branch as well and send over a PR when it's done. I'll let you decide whether or not you like it or not. (this is mostly just helpful for me to digest everything so no offence taken if you decide not to go with it).

dsifford commented 8 years ago

Also, I noticed you use the below format for your for-loops

for (var i = 0, ilen = someObj.length; i < ilen; i++) {
  // do stuff
}

is this just to squeeze the last couple nanoseconds out of the program or? Reason being, (IMHO) I think what's gained in speed, is lost in readability.

If you're digging the extra speed boost, perhaps write it like this?

let i = 0;
const ilen = someObj.length
for (i; i < ilen; i++) {
  // do stuff
}

Just a thought

dsifford commented 8 years ago

Oh, and one other tiny thing (I hope you aren't taking this as me tearing apart your code, these are totally just friendly suggestions).

It is my understanding that there really isn't a difference between the following in modern browsers (the same is true for getAttribute).

element.setAttribute('id', 'someId');

and

element.id = 'someId';

I'd suggest maybe going with the less verbose version for readability.

fbennett commented 8 years ago

Not at all, comment away.

Re for-loops, I used to catch the length in a var before the loop, but have found putting it on a single line more readable (for me). What's the other syntax?

On attributes vs properties, I've sometimes come across values (with input nodes) where the values differed (I've never fully grasped the relation between the two, to be honest).

dsifford commented 8 years ago

What's the other syntax?

The syntax that I use that is, admittedly, marginally less fast (0.1 milliseconds I believe was the average difference for objects with the length of 10 million) is the following:

for (let i = 0; i < someObj.length; i++) {
  // do stuff
}

I tested it on jsperf a while ago. Don't quote me on the exact speed difference.

On attributes vs properties, I've sometimes come across values (with input nodes) where the values differed

Not sure if I'm understanding. Are you saying you came across instances where an HTMLInputElement value was not in sync with the DOM? That's a legitimate concern. I suppose I haven't really had to worry about that as of recently since I've been pretty much all-in with React.

dsifford commented 8 years ago

Question:

Related to this:

/**
     * Initializes the processor, optionally populating it with a
     *   preexisting list of citations.
     *
     * @param {string} styleName The ID of a style
     * @param {string} localeName The ID of a locale
     * @param {Object[]} citationByIndex An array of citation objects with citationIDs
     * @return {void}
     */
    callInitProcessor(styleName, localeName, citationByIndex) {
        this.debug('callInitProcessor()');
        this.config.processorReady = false;
        if (!citationByIndex) {
            citationByIndex = [];
        }
        this.worker.postMessage({
            command: 'initProcessor',
            styleName,
            localeName,
            citationByIndex,
        });
    }

I noticed there is a check here for citationByIndex. What would it be if it isn't an array of objects?

fbennett commented 8 years ago

In an earlier iteration of the code, I think I had a call that dropped that argument, but it was replaced with one that uses my safeStorage value, so that page reload works correctly. As far as I can tell, those three lines are now redundant.

dsifford commented 8 years ago

Got it. I'll remove it.