-
Notifications
You must be signed in to change notification settings - Fork 125
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
Conversation
It won't show if we minify
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct!
There was a problem hiding this 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? 🪓☕️
There was a problem hiding this comment.
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?
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. |
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
andsinon
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.