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

Modernize the library #134

Merged
merged 6 commits into from
Jun 16, 2023
Merged

Modernize the library #134

merged 6 commits into from
Jun 16, 2023

Conversation

josefarias
Copy link
Contributor

@josefarias josefarias commented Jun 15, 2023

local_time hasn't been updated in 5 years. A lot has changed since then. This PR modernizes the library so that it can be built on modern hardware — including M2 macs.

Changes made

Remove Blade

Blade is a very nice tool. But it doesn't reflect the modern ecosystem.
It's built for sprockets, and includes some dependencies that won't build on M2 macs.
We're replacing it with modern alternatives that have more community support.

Replace Blade's buildtool capabilities with Rollup

This change is responsible for most diffs in this PR.
Sprockets (through Blade) made sure the LocalTime object was globally available.
This is no longer the case, so we have to import it everywhere it's needed.
This goes for library and test files.

We also can no longer import whole directories, so index.coffee files were created to be imported instead.

Replace Blade's test runner capabilities with express

Blade knows how to run QUnit tests inside a browser.
It builds a QUnit-ready document and serves it using Rack.

Now, we're serving our own QUnit-ready document using express.
Like with Blade, navigating to said document is enough to run the tests.

Update dependencies

moment and sinon needed upgrades in order to run with the new plumbing setup.
The old version of rails we were using had dependencies that didn't build on M2 macs.
All three dependencies are only used in tests — so the big jump in versions won't be a problem as long as the tests run. Which they do.

We also need a newer version of ruby to go with rails 7. Going with 2.7.8 as a stopgap, since ruby 3 was causing some failures.

Changes to README

I created a Contributing section to explain how to get started and run tests. They run practically the same way as they did with Blade, but I thought this could use some explaining.

Update Rake tasks

With these infrastructure changes, the rake tasks needed to be updated in order to still work.
These are still the preferred entry point for compiling assets and running tests.
Although it's still possible to use yarn to compile and test the javascript assets exclusively.

@josefarias josefarias requested a review from packagethief June 15, 2023 21:03
It won't show if we minify
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a large LOC difference with the previously included version of the library. Not sure if we previously only included the fake timers portion of the library or if Sinon has grown considerably since then.

Copy link
Member

Choose a reason for hiding this comment

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

This is for testing only, so not a concern regarding the host apps, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct!

Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Good job @josefarias!!

I understand that we are decaffeinating separately, right? 🪓☕️

Copy link
Member

Choose a reason for hiding this comment

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

This is for testing only, so not a concern regarding the host apps, right?

@josefarias
Copy link
Contributor Author

Good job @josefarias!!

I understand that we are decaffeinating separately, right? 🪓☕️

Thanks for the review @jorgemanrubia!

That's the plan. We have some other minor improvements in mind before we get to that, though. Might be a while before we decaf depending on how that goes.

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