Add initialize() hook to packages #13358

Merged
merged 1 commit into from Dec 1, 2016

Projects

None yet

5 participants

@matthewwithanm
Member

This adds a new hook, initialize() to packages. This is meant to be called before any interaction with the package. Like activate(), it's passed the serialized state from the previous session. Unlike activate(), it's called before deserialization, enabling item deserialization that's dependent on serialized package state.

It might be a little weird that a function called "initialize" can be called multiple times…open to bikeshedding the name!

@nathansobo
Contributor

I think we should maintain some state so that we call initialize once and only once.

@matthewwithanm
Member

Do you mean checking mainInitialized at each callsite instead of inside initialize()? I was thinking that doing it in that one location would make it less likely to be forgotten.

@maxbrunsfeld
Contributor

Great, simple solution. I left a few minor comments. Thanks @matthewwithanm!

spec/package-spec.coffee
@@ -205,3 +205,22 @@ describe "Package", ->
it "uses the package name defined in package.json", ->
expect(metadata.name).toBe 'package-with-a-totally-different-name'
+
+ describe "the initialize() hook", ->
+
@maxbrunsfeld
maxbrunsfeld Nov 30, 2016 Contributor

Could you remove this blank line?

spec/package-spec.coffee
+ pack.load()
+ expect(pack.initialize).not.toHaveBeenCalled()
+ atom.deserializers.deserialize({deserializer: 'Deserializer1', a: 'b'})
+ expect(pack.initialize).toHaveBeenCalled()
@maxbrunsfeld
maxbrunsfeld Nov 30, 2016 Contributor

Could you add an assertion that initialize only gets called once?

@matthewwithanm
matthewwithanm Nov 30, 2016 Member

This is actually testing the initializeIfNeeded() method (which could be called multiple times, but you're right, I should test the main module's instead and make the assertion. 👍

src/package.coffee
@mainActivated = false
+ initialize: ->
@maxbrunsfeld
maxbrunsfeld Nov 30, 2016 Contributor

I think we should rename this method to initializeIfNeeded, just to make it clear that we only call initialize once.

spec/package-spec.coffee
+ it "gets called when the package is activated", ->
+ packagePath = atom.project.getDirectories()[0].resolve('packages/package-with-deserializers')
+ pack = buildPackage(packagePath)
+ spyOn(pack, 'initializeIfNeeded').andCallThrough()
@maxbrunsfeld
maxbrunsfeld Nov 30, 2016 Contributor

Now that you're spying on the main module's initialize method, I think you should remove this spy on initializeIfNeeded, because it's an implementation detail.

@matthewwithanm
matthewwithanm Nov 30, 2016 Member

I thought maybe it would be good to make sure that it wasn't called when loading the main module…but I could remove it or break it out into another test too.

@matthewwithanm matthewwithanm Add initialize() hook to packages
25518b9
@nathansobo
Contributor

@damieng I'm assuming the AppVeyor x64 failure is unrelated?

@damieng
Member
damieng commented Dec 1, 2016

Yeah sorry there are 3 tests that are flakey on x64 right now. I've been trying to repro and get to the bottom of them here but only managed to reproduce 1 of them locally and even that is rare. Looks like possibly race condition based. I'll stop them running on Windows for now.

@nathansobo
Contributor

@damieng Personally I'm cool with you leaving them on if you're in the process of addressing it and turning them off would be a hinderance.

@nathansobo nathansobo merged commit 4716ad8 into master Dec 1, 2016

4 of 5 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@nathansobo nathansobo deleted the fb-mdt-initialize-hook branch Dec 1, 2016
@nathansobo
Contributor

@matthewwithanm Thanks for the clean work here and documenting it too.

@matthewwithanm
Member

Thanks for the help 😊

@abe33
Contributor
abe33 commented Dec 2, 2016

Hi there, just noticed this change this morning as it started breaking one of my package: abe33/minimap-pigments@f78ed88

The thing is: if a package already has a initialize function whose purpose is different it can lead to errors (which was my case). I'm not sure what would be the best solution in that case, maybe I'm the only one that had a initialize function in the main module, but maybe it would be worth making sure it won't break too many packages.

@nathansobo
Contributor
nathansobo commented Dec 2, 2016 edited

Yeah this was an oversight. We're doing a search now to look at the potential damage and may need to iterate on this if it's too many packages. I fear it could be a lot. Thanks for the report.

@nathansobo
Contributor
nathansobo commented Dec 2, 2016 edited

Here's a list of packages defining an initialize method on their main module, sorted descending by download count:

package download count PR or issue
editor-background 7463 downloads camel-chased/editor-background#33
project-ring 6502 downloads vellerefond/project-ring#50
notebook 2337 downloads skulled/notebook#15
tree-view-search-bar 1263 downloads lixinliang/tree-view-search-bar#7
scroll-searcher 846 downloads Aakash1312/scroll-searcher#14
atom-trello 837 downloads sgengler/atom-trello#4
atom-bugs 795 downloads willyelm/atom-bugs#11
punchclock 670 downloads queenp/punchclock#21
wordpress-suite 566 downloads peterjohnhunt/wordpress-suite#4
atom-gist 460 downloads mananshah99/atom-gist#10
timekeeper 303 downloads skulled/timekeeper#8
autocode 178 downloads ctate/autocode-atom#1
btom-mode 52 downloads bouzuya/atom-btom-mode#2
repository-file-actions 30 downloads vellerefond/repository-file-actions#2

We're going to open PRs on anything with over 800 downloads and open issues on the rest.

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