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

fix: Incorrect path to fetch the routeAfterAuthentication property in the config object #2215

Conversation

LuisAverhoff
Copy link
Contributor

@LuisAverhoff LuisAverhoff commented Jun 12, 2020

Simple fix to properly get the routeAfterAuthentication property from the config object upon application load.

@LuisAverhoff LuisAverhoff force-pushed the fix/configuration-routeAfterAuthentication branch from da8bb23 to d2efa60 Compare June 12, 2020 23:39
@LuisAverhoff LuisAverhoff changed the title fix: Incorrect path to fetch the routeAfterAuthentication property in the … fix: Incorrect path to fetch the routeAfterAuthentication property in the config object Jun 12, 2020
@LuisAverhoff LuisAverhoff force-pushed the fix/configuration-routeAfterAuthentication branch from e1ccbc9 to ea46ba2 Compare June 12, 2020 23:59
@LuisAverhoff LuisAverhoff force-pushed the fix/configuration-routeAfterAuthentication branch from ea46ba2 to 3dd53fc Compare June 13, 2020 00:04
@marcoow
Copy link
Member

marcoow commented Jun 17, 2020

I think this might be a misunderstanding – we actually want the configuration to be

'ember-simple-auth': {
  routeAfterAuthentication: '/some-route'
}

in config/environment.js. Scoping an addon's config to its name prevents clashes with other addons that might have the same settings.

@LuisAverhoff
Copy link
Contributor Author

LuisAverhoff commented Jun 18, 2020

@marcoow I made this pr because in the app folder of the addon for the initializer, it is grabbing the ember simple auth config and not the entire ENV config object(which makes sense to me). If it was the latter, then ember-simple-auth.routeAfterAuthentication would be correct but since you are grabbing the config for ember simple auth, to make it work currently you have to do

'ember-simple-auth':{
    'ember-simple-auth':{
        'routeAfterAuthentication': 'some-route'
     }
}

I could update the initializer to just use the entire Env config if that is what you were going for.

Here is the code in question

app/initializers/ember-simple-auth.js

import ENV from '../config/environment';
import Configuration from 'ember-simple-auth/configuration';
import setupSession from 'ember-simple-auth/initializers/setup-session';
import setupSessionService from 'ember-simple-auth/initializers/setup-session-service';
import setupSessionRestoration from 'ember-simple-auth/initializers/setup-session-restoration';

export default {
  name: 'ember-simple-auth',

  initialize(registry) {
    const config = ENV['ember-simple-auth'] || {};
    config.rootURL = ENV.rootURL || ENV.baseURL;
    Configuration.load(config);

    setupSession(registry);
    setupSessionService(registry);
    setupSessionRestoration(registry);
  }
};

Notice how you are already grabbing the ember simple auth config which means you dont need to reference the key ember-simple-auth again in the getter unless I'm missing something.

@LuisAverhoff
Copy link
Contributor Author

LuisAverhoff commented Jun 18, 2020

@marcoow Tell me if this is more what you were going for.

app/initializers/ember-simple-auth.js

import ENV from '../config/environment';
import Configuration from 'ember-simple-auth/configuration';
import setupSession from 'ember-simple-auth/initializers/setup-session';
import setupSessionService from 'ember-simple-auth/initializers/setup-session-service';
import setupSessionRestoration from 'ember-simple-auth/initializers/setup-session-restoration';

export default {
  name: 'ember-simple-auth',

  initialize(registry) {
    Configuration.load(ENV);

    setupSession(registry);
    setupSessionService(registry);
    setupSessionRestoration(registry);
  }
};

addon/configuration.js

import { getWithDefault } from '@ember/object';

const DEFAULTS = {
  rootURL: '',
  routeAfterAuthentication: 'index',
};

/**
  Ember Simple Auth's configuration object.
  @class Configuration
  @extends Object
  @module ember-simple-auth/configuration
  @public
*/
export default {
  /**
    The root URL of the application as configured in `config/environment.js`.
    @property rootURL
    @readOnly
    @static
    @type String
    @default ''
    @public
  */
  rootURL: DEFAULTS.rootURL,

  /**
    The route to transition to after successful authentication.
    @property routeAfterAuthentication
    @readOnly
    @static
    @type String
    @default 'index'
    @public
  */
  routeAfterAuthentication: DEFAULTS.routeAfterAuthentication,

  load(config) {
    this.rootURL = config.rootURL || config.baseURL || DEFAULTS.rootURL;
    this.routeAfterAuthentication = getWithDefault(config, 'ember-simple-auth.routeAfterAuthentication', DEFAULTS.routeAfterAuthentication);
  }
};

@LuisAverhoff
Copy link
Contributor Author

LuisAverhoff commented Jun 19, 2020

@marcoow Based on your comment, I reverted back how the test was and passed in the Entire ENV config object in the ember-simple-auth.js initializer file.

@marcoow
Copy link
Member

marcoow commented Jun 19, 2020

Ah, sorry my comment was wrong of course and what you were saying initially is completely correct – this is in fact a bug. Can you remove the last commit though? I think the original fix is better. Sorry for the confusion!

@marcoow marcoow self-requested a review June 19, 2020 13:38
@LuisAverhoff LuisAverhoff force-pushed the fix/configuration-routeAfterAuthentication branch from 744bd5a to 3dd53fc Compare June 19, 2020 15:24
@LuisAverhoff
Copy link
Contributor Author

LuisAverhoff commented Jun 19, 2020

@marcoow Alright, I've reverted to the previous commit as you asked. Pull request should be ready to go.

Copy link
Member

@marcoow marcoow left a comment

Choose a reason for hiding this comment

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

thanks!

@marcoow marcoow merged commit 3ac64c3 into mainmatter:master Jun 19, 2020
@marcoow marcoow added the bug label Jun 19, 2020
@bitwolfe
Copy link

@marcoow Any chance we could get a new release with this fix?

@LuisAverhoff LuisAverhoff deleted the fix/configuration-routeAfterAuthentication branch October 1, 2020 18:14
@rjschie rjschie mentioned this pull request Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants