search

lddubeau / bootprompt

Bootstrap powered alert, confirm and flexible dialog boxes
https://lddubeau.github.io/bootprompt/
Other
12 stars 3 forks source link

Issues encountered during forking #1

Closed lddubeau closed 5 years ago

lddubeau commented 5 years ago

@tiesont This repo contains the WIP on my fork of Bootbox.

I can make you a collaborator on this repo if you want. That's entirely up to you.

(If you look around the commits you'll see I first named the fork "bootshine", but after a while I found I hated that name, and then renamed again to "bootprompt".)

As I mentioned, I found that over 50 tests did not run in your branch. So when I turned them on, I found some failing tests and fixed the problems. You know much more than I do about the internals so you may have comments about my choices. I'm going to point out the relevant commits below.

  1. https://github.com/lddubeau/bootprompt/commit/9b953d720a60b03f0b57aea818ca1b968741ba49 This fix brings the code in conformance with what the test suite expected. But more importantly, it provides a nicer message to the user. Without the change, the user would get the error about min having to be lower than max but the real problem is that the other number is NaN.

  2. https://github.com/lddubeau/bootprompt/commit/ba1d9c21e5d2e298d79f9b0ee64df216576858ff The test did not take into account the behavior of a range that has min-max unset. See comment in the commit.

  3. https://github.com/lddubeau/bootprompt/commit/65e24e82500c8a95b615c3ed164ea6ecc1c5188a This is similar to the previous commit in that it fixes the test suite so that its expectations conform to the specs that determine how browsers are supposed to behave.

  4. https://github.com/lddubeau/bootprompt/commit/fc3a3e7fdd4bf8f118d94fe319b5ac0be244e2b9 This test failed before I made this commit. I infer from the test that the intent is that if the user does not specify a default value, then first inputOption should determine the default. However, browsers are such that if the user does not act on the select element, then its value is never changed and remains what it was at creation, an empty string.

The cases above were disconnects between the code and the test suite, but I found an additional issue. I don't think there's a specific line in Bootstrap's documentation that says so, but the de facto standard that I've seen in constructing modals is to take the button which has the class btn-primary as the "default action" for the modal. Being the default action, it should be the button that is focused when the modal shows up. In Bootbox 4, it worked that way. So I set up my modals with btn-primary for the button which is safest. For instance, if someone is about to overwrite data, my app would ask "Do you want to overwrite this data?" and "No" would be the btn-primary. If the user just hits ENTER after the modal shows up, the data has not been overwritten and the user can try again if they really meant to overwrite the data.

On your branch, this is flipped around because the focus is given to the bootbox-accept button. After I started using your branch, all the modals I had that would ask for confirmation before doing something risky would focus the button which makes irreversible changes. In the example I gave above, "Yes" would be focused and if the user just hit ENTER, they'd do the irreversible change. (Yes, there are backups but it's a pain to have to go to a backup to recover the data.)

So in my fork I've changed the code back to focus btn-primary instead of bootprompt-accept.

tiesont commented 5 years ago

The reason I added the bootbox-accept class and made that the default button to trigger was that there is no guarantee that there is a button with the .btn-primary class attached to it - you can override the className value on any button. It might make more sense to add a defaultButton property (call it whatever you think makes sense), and have that be what gets triggered automatically.

I haven't had time to look at the rest of the code, but it sounds like you know what you're doing. It's up to you if you would like to add me as a collaborator. I was originally added to the bootbox repo as a volunteer to update the docs (which were fairly out-of-date at the time), which eventually morphed into bug/issue triage and then "primary caregiver". If you think there's something I could help with, then by all means ask.

Thank you for your effort on this, regardless.

lddubeau commented 5 years ago

You're right that someone customizing the classes may end up with no .btn-primary. I guess there could be an additional option to point to a button as the default one, but I prefer not to add more options for the time being.

tiesont commented 5 years ago

Can I ask which repo you forked from, and/or when? I haven't pushed it to the main repo's 5.x branch yet, but someone did ask for the ability to have a select with the multiple property enabled (see https://github.com/tiesont/bootbox/issues/8), which was relatively simple to implement. I can port that code here if you think it's worth having.

lddubeau commented 5 years ago

I merged 5f506b2bffbd190ec42ed589c013dc0a983bb345 from the v5.x branch into master just before forking. It does not include the code for multiple. It is worth having support for it but the code base here has changed a lot since the fork. The source of the library is now in TypeScript, and I've refactored some of the code (for instance, the prompt function was looooooooooooooooong, and I've refactored it into a series of smaller functions). So it's not just a matter of moving the patch to this repo. git will most likely balk with a hairy merge conflict. At this point, the patch would have to be essentially hand-translated to something that works with this repo. If that's something you want to do, then I would welcome the PR. Otherwise, if you point me to the relevant commits in your repo, I can do the work. It probably would not take that much time to convert it.

You may be wondering about the reason for the conversion to TypeScript. Here they are.

Better TypeScript support. I used bootbox (and now use bootprompt) with projects that are written in TypeScript. So TypeScript support is essential for me. With bootbox, one could install @types/bootbox and get the TypeScript declarations this way. From the point of view of someone consuming bootbox, the problem is mostly solved. However, there are remaining issues. These are not issues specific to @types/bootbox but common to most @types packages, because the folks producing @types/foo packages are usually not the same folks producing foo.

  1. @types/bootbox is sometimes lagging behind bootbox. The folks producing @types/bootbox need to notice a new bootbox release, fix the declarations, release, etc.

  2. The fact that the folks producing the declarations are not the same as the folks developing the library sometimes cause bugs in the declarations. The folks producing the declarations misunderstand what types a function accepts or returns and produce incorrect declarations.

It is possible for a developer who does not want to convert an entire project to TS to support TS by just bundling declaration files with the library. So I could have just created TS declaration files to be bundled with bootprompt. Someone installing it would then get the declarations together with the library. In theory this eliminates the issues I mentioned above. In practice, the issues may remain. When adding new options, functions, features, it is easy to forget to update the declaration files. (Here is a case. In that project I decided to not convert the whole project to TS, and just added TS declaration files next to the JS. I made a change and forgot to update the declaration files. Then a user found the problem, and had to produce a PR to fix it. Oops.)

The most robust way to produce declaration files is to convert the code base to TS and let the TS compiler produce the declaration files. This way the files are always in sync with what the library expects and produces.

A more pleasant programming language. bootbox is essentially coded in ES5. I wrote a lot of code in ES5 but I've moved on. Now, when I type JS, I tend to type code that needs features of ES6, 7, etc. I tend to type let foo or const foo rather than var foo, I type () => {} for functions that don't need to have their own value for this instead of function () {}, I type for (... of ...) for loops that iterate over elements of arrays and array-like structures (instead of having to maintain a pointless index), and use async / await instead of new Promise(...).then(...). ES6 and later versions of the language are just so much more pleasant to use than ES5. At the same time bootprompt has run on browsers that don't support ES6, 7, etc. So if the source is going to use features that are not in ES5, the code has to be downcompiled to ES5. TypeScript provides this downcompilation. I could use Babel instead of TS, and just keep the code in plain JavaScript. However, in my view, if I'm going to introduce a compiler into the build process, might as well get as much millage as possible out of it: with TS, I get downcompilation, type checking, and declaration files. With Babel, I get only downcompilation.

And parallel to all this, a bonus. When I converted the code base to TS, the compiler found a few bugs for me. (Babel by itself would not have found these bugs, as they depend on type checking.)

tiesont commented 5 years ago

Interesting. The last work I did on my fork (not pushed anywhere) was to do the same: convert the source to TypeScript, pretty much for the same reason you're describing. I also thought about seeing how much jQuery code I could remove, so that I could use Bootbox in an Angular project that uses a "no jQuery" version of Bootstrap.

Totally agree about the prompt function. It got even longer when I added the input types that are new to 5.x, since I also added support for more attributes/properties (min, max, etc.). Sounds like you're pretty much doing what I really should have been doing for 5.x. I was (at the time) more hopeful on getting an official 5.x release, though, and thought keeping my changes limited to Bootstrap 4 compatibility plus a few extra features would make that easier. Clearly, that wasn't the case, as Nick's pretty much stopped participating (he had to shut down Finch around that time, so probably not the best time to be focusing on OSS projects).

I think the other Bootbox maintainer who tends to be active, @tarlepp, might be a useful resource for you, assuming he has time and the inclination to do so. @tarlepp - thoughts?

lddubeau commented 5 years ago

Yes, lessening the use of jQuery is also on my TODO list. Things like $.isFunction are probably no longer useful. I've just found it's been deprecated in jQuery 3.3!

tarlepp commented 5 years ago

Yeah, sure using something else than ES5 would be really nice, but I have couple of topics to talk about:

tarlepp commented 5 years ago

And let me make this clear - I'm not against this kind of stuff - quite opposite - but doing stuff like this will need some serious effort for maintainers - and not saying that we cannot do that but, eg. I have only done some small stuff for this library within my freetime - and really there isn't so much of that anymore...

lddubeau commented 5 years ago

Yeah, sure using something else than ES5 would be really nice, but I have couple of topics to talk about:

  • The refactor / test job for that is quite huge - or that is my first impressions about this change. Also what about people who are already using this? So at least it must be major version change.

I've decided to keep the version history intact and start this fork at 5.0.0.

  • Also I think that bootstrap still requires jQuery, so why would we be using that too?

Bootstrap requires a library that provides a subset of the full jQuery API. You can use jQuery slim with it, it is still jQuery, but not the full jQuery API. There are also other jQuery-ish libraries out there like Zepto or jqLite. If Bootstrap works fine with Zepto, for instance, it would be a shame if bootprompt had a problem with that and force the library user to use jQuery.

  • And what about browser support? Not all browsers still support those ES6/7 features - and yes we could make a separated build process to handle this.

Done. The build process downcompiles to ES5 as we speak.

  • And sure old-fashion jQuery stuff isn't so "fancy" anymore - but it works - why to change something that isn't broken?

Some usages of jQuery are not worth it. There's no good reason to do $.isFunction(foo) rather than typeof foo === "function". None. One of the many problems with jQuery is that sometimes it tries to be helpful in ways that are actually harmful. If I see $.isFunction(foo) in a piece of code, I cannot be sure that it won't try to do something seemingly clever but ultimately problematic... unless I go read the documentation or the source code of jQuery. And I cannot be sure that the next version won't try to do something clever but harmful.

Another problem with relying on jQuery is that it slows down the code. Granted I doubt that anyone is going to use bootprompt to display a thousand modals in a tight loop, but on principle, I don't want to the bits of jQuery that I can trivially do without.

And let me make this clear - I'm not against this kind of stuff - quite opposite - but doing stuff like this will need some serious effort for maintainers - and not saying that we cannot do that but, eg. I have only done some small stuff for this library within my freetime - and really there isn't so much of that anymore...

I've taken upon myself to implement everything we've discussed in this issue (and it's all been pushed to the master branch). As we speak what's been discussed here is probably around 90% implemented code-wise. I also need to go through the documentation.

tiesont commented 5 years ago

Not sure how/where else to post this, and a new issue doesn't seem appropriate, so:

I went ahead and pushed a 5.0 version out to the "upstream" repo. You probably won't get much use out of it, but I did make a few code changes (mostly to how min and max values are validated), fixed the unit tests (thanks mostly to you, since your explanations helped me finally see what was broken), and updated the crap out of all the npm packages which the repo references.

I also updated and rewrote the docs (a little while back) to use PrettyDocs (https://themes.3rdwavemedia.com/demo/prettydocs/) as a base - that's what you'll see on http://bootboxjs.com now. Feel free to replace what you found in the /docs folder with that (although there is at least one new option you might be missing). You'll find the docs in the gh-pages branch, if you're interested.

One thing I was thinking we could do, if you're interested, is to create an organization for BootboxJS, and then a repo called BootboxJS (probably all lowercase). That would let us retain the "legacy" of the original name, and having an org makes it easier for users to come and go on the project. Of course, if coding in TypeScript and offering a compiled version makes more sense, then maybe go with BootboxTS. Or, wooo, Bootbot - I can see that leading to a pretty cool (or amusing?) logo/branding.

lddubeau commented 5 years ago

@tiesont The new docs you have look good but I don't think I'll be replacing the current docs with that. I'm seeing these issues with the current docs:

  1. Duplication. Changing the API means making a change in the source code, and making a change in the documentation. In my experience this leads to the documentation diverging from the code, then being fixed, then diverging again, then being fixed, and so on. The documentation is constantly playing catch up with the actual API. In theory it would be possible to record the API documentation with the source. Typedoc, for instance, could be used. There were a few bugs in the Bootbox documentation that I fixed when reworking it for Bootprompt. Some of them could not have happened if something like jsdoc3 (it is approximately to JS what Typedoc is to TS) would have been used to produce the API documentation.

  2. I want to move away from a setup in which I have to hand-code in HTML substantial parts of the documentation. I realize that good live examples will require some HTML hand-coding, and I'm fine with that, but I'd rather use Markdown or ReStructuredText to build lists, or tables, or create hyperlinks, rather than code everything by hand. When I reworked the documentation for Bootprompt, I did find some HTML coding mistakes. I'd like to claim that I'm great at writing HTML but I suspect that I'd make the same mistakes once in a while. Validation tools could catch mistakes but I'd rather prevent them from happening in the first place, and validation tools do nothing to reduce the verbosity of HTML.

Unless I'm mistaken, I don't think PrettyDocs does anything towards helping with these issues.

Moving Bootprompt under an organization is fine, but I'm not so keen on renaming it. I've already gone through the code and doc to rename things, and it's been released. I'd rather not go through that again.

tiesont commented 5 years ago

I believe you can use something like Jekyll to build your documentation from Markdown, provided you can set your Github repo up to use a /docs folder within master (as opposed to the gh-pages branch). I wanted (and still do) to move to Markdown for Bootbox, but (as we already know) only an admin can make that change, and, well...

No, PrettyDocs does nothing - it was just a theme + templates, for the most part, that seemed a bit cleaner than what you have now, which I wrote by hand (think I also found the errors you're referring to when I was porting it over).

Typedoc looks pretty nice. Definitely worth a bookmark. I also found https://www.mkdocs.org/, although that a) seems to require Python and b) seems to be just a Markdown converter, not something that creates Javadoc-like API documentation (which Typedoc does).

At any rate, rather than continuing to add to this issue, if you have anything you would like help with, or to discuss, you can contact me at the email shown on my profile at any time. Or, you could always setup something like https://gitter.im/, I suppose.

Thanks for humoring my suggestions, regardless.