-
-
Notifications
You must be signed in to change notification settings - Fork 48
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
Refactor initializer #33
Refactor initializer #33
Conversation
Fix the failing node linting, and also manually install ember-try
Ooh, that's an amazing PR @mydea, thank you very much! Will look into it shortly. |
addon/services/asset-map.js
Outdated
enabled: false, | ||
map: {}, | ||
map: computed(function() { |
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.
Small nitpick, could be as
map: computed(() => ({})),
I have added one more commit, fixing an issue I noticed when building for production. I also made the small adaption you requested, and disabled |
@RuslanZavacky Running this in production, works like charm! Care to cut a release on NPM? ;) |
Thank you, @mydea! Thats a great contribution ❤️ |
Awesome! Just noting, not sure if that is on purpose, but the 0.7.0 release is not on NPM yet? |
@mydea published now :) |
After upgrading to the new test syntax, the private functionality the initializer uses breaks.
Especially, this part doesn't work anymore in the new style tests:
This PR first updates all the dependencies to their latest versions, and then refactors the initializer to work with the new syntax.
This should be a completely internal change - to the end user, nothing should change.
It turned out, the hardest part was to get it to work with Fastboot. The old way of fetching that data from a meta tag didn't work there, as Fastboot doesn't have access to the DOM.
Because of this, I refactored it to instead write the URL/inline form into a dedicated file, and read it from there.
At the end, this should not change anything for a consumer of this addon.
One thing I figured out, is that if you're using Fastboot, you need to use the inline form, as Fastboot cannot use jQuery to fetch the assetMap. If it detects that you are building for Fastboot, it will automatically force you into
inline: true
mode, and say so on the console (so you know what's going on). Generally, I think it would be a good idea to recommend inlining it anyhow - it will be put into thevendor.js
file, and you save one request. Also, I found some issues with assetMap fingerprinting (e.g. ember-cli/broccoli-asset-rev#122, and also some issues with fastboot) - so inlining is definitely the safest option. The other option still works the same as before, though.