Activate the config for a package before displaying it

[Arcath]

Fixes #356 by calling Package.activateConfig() if the package is not yet active.

[thedaniel]

Sorry to let this sit for so long, will hopefully review soon.

[thedaniel]

This seems like the right thing to do, but I'm not sure about the visibility / stability of activateConfig - it's not documented, etc. I'm personally down with using it directly in a core package since I'm happy to keep settings-view updated to track API chances, but it sets an example for how other packages might want to interact with packages and we should consider whether it's an API we want to document & commit to for a year+ (or at least put a big warning PRIVATE API USE DON'T DO THIS AT HOME message in settings-view, which makes me feel a bit uneasy).

I personally think activateConfig is pretty useful here and would like to merge but but there must be a reason that Package isn't part of the public API. Maybe someone else on @atom/core has some feedback / knows more about why Package is private.

[nathansobo]

Currently the entire Package class is not in the public API, and when you click a link to it on the docs site it actually 404s 😱. Looking at the other settings view code, we're definitely reading a bunch of properties on the packages, which already violates our API policy of using getters for everything except in specific cases like the atom global. I don't think this makes the situation worse.

This really brings to light a problem with hanging our config off of the package's main module. The schema should really be a separate JSON file that can be read independently. It makes me somewhat nervous that this will cause us to require the main module of packages the user has deactivated. True, it won't call activate, but it will run code. Should we instead render in some kind of disabled state until the package is enabled? We should really fix our approach to the schema soon, but we will have to support this until 2.0.

[thedaniel]

While I think that 99% of the time, requiring the main module and calling activateConfig is fine, I think that this:

Should we instead render in some kind of disabled state until the package is enabled?

... is a good halfway point if we have to compromise. I'm nervous about running unactivated package code as well, but I feel like modifying editor state upon require is such egregiously bad practice that if it does happen it will be quickly noticed / complained about / fixed by package authors.

[nathansobo]

After sleeping on this, I really think we should set a good precedent and only require the main module when a package is actually active. It seems like not a huge deal right now, but reading and evaluating JS is a pretty nasty synchronous dependency to bake into expectations around simply rendering a config UI.

Would anyone be interested in helping to move the config schema stuff to a separate file? We would still need to support config on the main module. In that case, I think we should show a disabled UI in the settings view. That will motivate people to move their config off the main module.

[Arcath]

Would it not make sense to apply this for now so that Packages work as expected whilst the Package API is changed?

[nathansobo]

@kevinsawicki Can you chime in on this? I'm unsure how to proceed.

To summarize: In order to display a config UI for inactive packages, we need to require the main module in order to retrieve their config schema. This demonstrates that config schemas shouldn't be on the main module, but in a separate file. But they are in the main module right now. So the question is, should we require the main module to activateConfig just for the settings view UI as this PR does?

I suppose we could always change the policy later when we add config schemas as separate files to show a disabled view for packages that don't offer an external config schema.

[thedaniel]

FWIW I've come around to "deactivated packages should never load code" and changed my mind on activateConfig(). It seems like a sensible promise to make to users and developers.

[kevinsawicki]

Just to give some context, this type of code (calling activateConfig) used to be in the SettingsView.

It looks like it was removed here though I'm not sure if it was intentional or not:

2a1ca7e#diff-b7fdec02de14288c9a9e402b91ab5122L38

2a1ca7e#diff-b7fdec02de14288c9a9e402b91ab5122L215

So since it used to do this, I'd prefer to just resurrect how it was done since that process seemed to work for a long time, unless I'm missing a new wrinkle.

[kevinsawicki]

I do think pushing the config schema into the package.json file would be nice to have though

[nathansobo]

@thedaniel I'm happy enough to follow @kevinsawicki's lead on this. Do you want to merge this at your discretion?

[thedaniel]

I want to be dogmatic about it but I concede. @Arcath can you add a comment near the activateConfig call indicating it's a private api and not appropriate for use in 3rd party packages? With that I'll merge.

[kevinsawicki]

I think this should be done in a next tick (like it used to be) since activating the config for ~100 packages will probably take some time and at least the settings view will get a chance to render and "feel" like it is continuing to load while the configs are activating.

[Arcath]

This is on the installed-package view so its only loading the package you've opened not all of them.

[Arcath]

Just added it now, sorry for taking so long I've been off ill the last week.

[izuzak]

This PR is no longer mergeable. Looking at the last few comments in this thread it seems that @thedaniel wanted to merge this is @Arcath made a change. That change was made by @Arcath a month ago but it seems that some other changes on master made the PR unmergeable. @thedaniel -- would you still be 👍 on merging this if the merge conflicts are resolved?

[thedaniel]

@izuzak yeah, this can merge if the conflicts are resolved. We still need to make config static at some point but this is confusing to users in master right now.

[nathansobo]

I went ahead and resolved the merge conflict. @kevinsawicki it would be really great to get a release out for this tomorrow or Tuesday in time for my workshop, because not loading the configuration until activation represents a hurdle I'd rather avoid explaining to people.

[thedaniel]

@nathansobo when's your workshop? This broke viewing details for not-yet-installed packages so you're probably going to need another release before you talk to a class.

[nathansobo]

I'm just going to remove activation commands for the package were building and make it a non-issue. Sorry for the breakage.

[thedaniel]

No reason to apologize, I should have looked more closely before 👍 - also this wasn't broken in a release, just on master (i was mistaken)

[kevinsawicki]

@kevinsawicki it would be really great to get a release out for this tomorrow or Tuesday in time for my workshop

Was planning on releasing in the next hour or so

[kevinsawicki]

Was planning on releasing in the next hour or so

0.191 is now out