-
Notifications
You must be signed in to change notification settings - Fork 53
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
Replace hash-obj w object-hash #85
Conversation
package.json
Outdated
"cover": "mastarm test --coverage", | ||
"jest": "mastarm test", | ||
"lint": "mastarm lint lib --quiet", | ||
"lint-docs": "documentation lint lib/**/*.js", | ||
"prepublish": "mastarm prepublish --config configurations/prepublish", | ||
"prestart": "yarn", | ||
"test": "yarn run lint && yarn run lint-docs && yarn run jest", | ||
"start": "mastarm build --serve example.js" | ||
"start": "mastarm build --serve example.js", | ||
"zip": "zip -r dist.zip index.html example.css dist" |
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.
I'm pretty sure the zip command won't work on windows. What is the purpose of this script?
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.
We can remove this. I just wanted to preserve how to deploy the example. Thoughts on an alternative approach?
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.
Awaiting feedback on #85 (comment).
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.
Approving now that zip package.json script is removed.
.gitignore
Outdated
@@ -14,6 +14,7 @@ coverage | |||
|
|||
# built example app | |||
dist | |||
dist.zip |
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 can probably be removed too.
Codecov Report
@@ Coverage Diff @@
## dev #85 +/- ##
========================================
- Coverage 6.37% 6.36% -0.01%
========================================
Files 134 134
Lines 5996 5997 +1
Branches 1750 1750
========================================
Hits 382 382
- Misses 4746 4747 +1
Partials 868 868
Continue to review full report at Codecov.
|
These changes now make otp-rr work on IE 11. This PR is dependent on conveyal/mastarm#279 and references a commit in mastarm in order for things to work temporarily. Unfortunately, this solution requires the import of |
Assigning back to you, @landonreed. Let me know if you think this'll work for now. |
What about building the library |
We'd also need to add es6-math into trimet-mod-otp. The import order of things would matter, so it's best to require the polyfill from the root file. |
Gotcha. Ideally mastarm would just work out of the box, but for now this should be good enough. Let's just keep a ticket open in mastarm referencing this solution until we come up with something better. |
🎉 This PR is included in version 0.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fix #84 by replacing hash-obj with object-hash (which is transpiled).
This also updates the example app and makes it IE compatible, which now makes it useful for quickly deploying to test whether the library itself is transpiled/polyfilled correctly (deployed at https://trimet-mod-staging.ibi-transit.com right now), rather than having to bundle through the TriMet app.
Note: this is also waiting on changes from
conveyal/mastarm#273conveyal/mastarm#277