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

Refactor initializer #33

Merged
merged 4 commits into from
Mar 12, 2018
Merged

Refactor initializer #33

merged 4 commits into from
Mar 12, 2018

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Feb 16, 2018

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:

 const container = app.__container__; 
  let config; 
  if(container.factoryFor) { 
    config = container.factoryFor('config:environment').class; 
  } else { 
    config = container.lookupFactory('config:environment'); 
  } 

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 the vendor.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.

Francesco Novy added 3 commits February 15, 2018 14:02
Fix the failing node linting, and also manually install ember-try
@RuslanZavacky
Copy link
Contributor

Ooh, that's an amazing PR @mydea, thank you very much! Will look into it shortly.

enabled: false,
map: {},
map: computed(function() {
Copy link
Contributor

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(() => ({})),

@mydea
Copy link
Contributor Author

mydea commented Feb 21, 2018

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 isDevelopingAddon.

@zeppelin
Copy link

zeppelin commented Mar 8, 2018

@RuslanZavacky Running this in production, works like charm! Care to cut a release on NPM? ;)

@RuslanZavacky RuslanZavacky merged commit dfccc56 into adopted-ember-addons:master Mar 12, 2018
@RuslanZavacky
Copy link
Contributor

Thank you, @mydea! Thats a great contribution ❤️

@mydea
Copy link
Contributor Author

mydea commented Mar 13, 2018

Awesome! Just noting, not sure if that is on purpose, but the 0.7.0 release is not on NPM yet?

@RuslanZavacky
Copy link
Contributor

@mydea published now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants