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

Make htmx IE11 compatible again + tests IE11 compatible #1687

Merged
merged 7 commits into from
Sep 6, 2023

Conversation

Telroshan
Copy link
Collaborator

@Telroshan Telroshan commented Aug 9, 2023

I know IE11 compatibility is almost a meme at this point, but as htmx 2 isn't that far ahead and with it the official end of IE11 compatibility, let's stick to the gimmick til the end!

Core changes:

  • Htmx is currently using mocha 10, that dropped support for IE11 in this release
    • Downgraded to mocha 9.2.2
  • IE doesn't support startswith / endswith on strings
    • Added startsWith/endsWith utility wrappers, using string.indexOf instead
  • IE doesn't support case insensitive query selectors
    • Added a try catch to fallback to standard (case sensitive) selectors instead of raising on error
  • IE doesn't support URL
    • Added a if statement to make a basic string comparison in that case, instead of raising an error
  • The loading-states extension was using a lot of IE11 incompatible syntax such as arrow functions or template literals
    • Replaced by IE11 compatible syntax (functions and strings concatenation)
  • The morphdom-swap extension was using fragment.firstElementChild, which IE doesn't support
    • Added a fallback to fragment.firstChild
  • IE11 doesn't support XPath

I also updated a lot of tests to be IE11 compatible:

  • IE may automatically remove empty class attribute + swap order between id and class, so we can't rely on a innerHTML equality check
    • Replaced with id / class / innerText equality checks
  • IE doesn't support some features (svg titles, form attribute, submitter property in event, templates)
    • Added skips for IE11 in those tests as we simply can't expect them to ever work in IE11
  • hyperscript isn't IE11 compatible
    • Added skips for IE11 in tests involving hyperscripts
  • mock-socket, used in the ws-ext tests, isn't IE11 compatible
    • Replaced it with a handmade socket mock, using the same syntax so the initial tests code didn't require changes (and could easily be replaced in the future with mock-socket again when we drop IE11 for htmx2)

How to test on IE11

Let me know if it'd be relevant to add a section on the repo's README for contributors, about how to test in IE11, as it seems pretty common that new features / commits break IE11 compatibility

@Renerick
Copy link
Collaborator

Renerick commented Aug 9, 2023

Yes, I added this https://github.com/thoov/mock-socket library to provide websocket mocks. At the first glance, it uses extends class syntax https://github.com/thoov/mock-socket/blob/master/src/websocket.js, which is not supported by IE https://caniuse.com/mdn-javascript_classes_extends. But maybe there is more issues

WebSockets could be tested by making our own mock and providing it via config.createWebSocket. In fact, this was the intended way initially, but I decided that ergonomics of the mocking library was well worth it.

If we were to make it IE 11 compatible, there is a couple ways:

  1. Get rid of the library and write mock object ourselves
  2. Vendor the library and alter it to get rid of the incompatible parts

@Telroshan
Copy link
Collaborator Author

Thanks for your input! Just realized the old attribute is indeed mocked with a handwritten system, so I might as well just do that... I'll investigate and update this PR when I get the chance

@Telroshan
Copy link
Collaborator Author

Should be good to go!
image

@alexpetros
Copy link
Collaborator

@Telroshan does npm run test work for you locally? Because the tests are failing here.

@Telroshan
Copy link
Collaborator Author

Telroshan commented Aug 21, 2023

Oh right @alexpetros, didn't even know that command existed to be honest, I was running npx serve and accessing the test page with both IE11 and my regular Chrome
I don't get errors in browsers, but indeed, npm run test raises errors locally... 4 on hx-boost attribute's tests
I have 4 failing though, I wonder why the runner has 27 of them 😮

Investigating!

@Telroshan
Copy link
Collaborator Author

Well, turns out running npm run test locally, on the current dev branch (i.e without any of my changes here), give me the same result : 4 failing tests, on hx-boost, all tests involing a a tag (forms seem to be ok)

@alexpetros did this happen before?

@alexpetros
Copy link
Collaborator

alexpetros commented Aug 24, 2023

What I can say is that when I pull the current dev branch, use node 15, and run npm t I get:

  616 passing (12s)
  1 pending

I know the test suite in headless mode is mildly finicky—it has a couple timing issues—but it's reliable enough that we can't really consider PRs that don't pass the CI.

I'll do what I can to help of course, this is a big PR and I'm grateful for you working on it.

@Telroshan
Copy link
Collaborator Author

I totally understand and agree that a PR not passing tests shouldn't be merged in.

Thanks for your results, now I have to figure out why neither of the branches pass the tests locally in my case... I mean It's not a "it doesn't work all the time" scenario, I constantly get those same 4 errors when running npm run test, whether it's on dev on my PR's branch, so there has to be something

I'll have to investigate some more, I'll probably start with the dev branch so my changes don't get in the way, but now I have at least the confirmation that what's happening for me on the upstream dev is absolutely not expected 😆

@alexpetros
Copy link
Collaborator

What's happening is definitely specific to your machine and very likely a timing problem with our tests; sucks that you have to be the one to figure that out, but that's just the limitation of the bandwidth that we have 😞

@Telroshan
Copy link
Collaborator Author

No worries, I'll figure it out eventually!

@Telroshan
Copy link
Collaborator Author

Telroshan commented Aug 24, 2023

So I uninstalled node (noticed I had node 16 and not 15...)

  • Installed node 15 (made sure no node nor npm was remaining)
  • Cloned htmx into a fresh new folder
  • Switched to upstream/dev branch
  • Ran npm i then npm run test
  • And somehow it... got even worse ?
572 passing (5s)
1 pending
44 failing

Kill me now


UPDATE :

  • Noticed the package.lock json had changed after my first npm install
  • So I reverted it
  • Ran npm i again
  • Ran tests again
  • ...I get once again the 4 errors mentioned above about hx-boost
    And that's a fresh new htmx clone 😢

@alexpetros
Copy link
Collaborator

This person had the same issue with the local test setup: #1650

Can you open a separate issue for the test stuff and we'll consolidate your findings there?

@alexpetros alexpetros added the bug Something isn't working label Aug 27, 2023
@Telroshan
Copy link
Collaborator Author

Yay tests finally pass on the pipeline with your fix in #1742 @alexpetros

@1cg 1cg merged commit 55c30b5 into bigskysoftware:dev Sep 6, 2023
1 check passed
@Telroshan Telroshan deleted the ie11-compatibility branch September 22, 2023 05:40
@Telroshan Telroshan mentioned this pull request Mar 17, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants