atom / archive-view

Open compressed files in Atom
MIT License
31 stars 32 forks source link

Decaffeinate remaining CoffeeScript files #56

Closed Alhadis closed 6 years ago

Alhadis commented 6 years ago

Note: the decaffeinate module played no part in the conversion. I did this manually, and I hate CoffeeScript enough to have legitimately enjoyed doing so. I'm happy to help you kill it off faster. 👍

Otherwise, there's nothing new being added in this PR, and certainly nothing pertaining to icon services. Review at your own pace.

Alhadis commented 6 years ago

I noticed the ArchiveView class isn't de/serialising properly... 😕

  1. Open spec/fixtures/file-icons.zip
  2. Run window:reload command
  3. Tab for ArchiveView missing in tab pane, and the following feedback written to console:
No deserializer found for
Object {path: "/Users/johngardner/Forks/Atom/ArchiveView/spec/fixtures/file-icons.zip"}
    deserialize @   deserializer-manager.js:77
    (anonymous) @   pane.coffee:29
    module.exports.Pane.deserialize @   pane.coffee:29
    deserialize @   deserializer-manager.js:75
    deserialize @   /Applications/Atom.app/Contents/Resources/app.asar/src/pane-container.js:54
    deserialize @   /Applications/Atom.app/Contents/Resources/app.asar/src/workspace-center.js:31
    deserialize @   workspace.js:366
    (anonymous) @   atom-environment.coffee:1015
50Wliu commented 6 years ago

I believe deserialization is broken because you forgot to specify the deserializer key (should be this.constructor.name). https://github.com/atom/timecop/blob/f7d954391cf3fde794c23dda2748c8cf37d9dec7/lib/timecop-view.js#L121

Alhadis commented 6 years ago

Ugh, I feel I should've known that already. 😢 Fixed, and thank you!

Alhadis commented 6 years ago

Alright, the specs are now passing and have been refactored to use async-spec-helpers.js, as per your recommendation. However, I'm seeing the following feedback when I open the spec-runner's dev-console:

WARNING: Leaking subscriptions for paths:
/Users/johngardner/Forks/Atom/ArchiveView/spec/fixtures/nested.tar

warnIfLeakingPathSubscriptions
    @ spec-helper.coffee:123
(anonymous)
    @ spec-helper.coffee:117
jasmine.Block.execute
    @ /Applications/Atom Beta.app/Contents/Resources/app.asar/vendor/jasmine.js:1070
jasmine.Queue.next_
    @ /Applications/Atom Beta.app/Contents/Resources/app.asar/vendor/jasmine.js:2102
onComplete
    @ /Applications/Atom Beta.app/Contents/Resources/app.asar/vendor/jasmine.js:2098
jasmine.WaitsForBlock.MultiCompletion.attemptCompletion
    @ /Applications/Atom Beta.app/Contents/Resources/app.asar/vendor/jasmine.js:2648
(anonymous)
    @ /Applications/Atom Beta.app/Contents/Resources/app.asar/vendor/jasmine.js:2658

Oddly, the feedback isn't emitted when running the specs headlessly.

50Wliu commented 6 years ago

This looks good to me from a general conversion standpoint. Since we're adding items to the DOM manually it doesn't look like we can easily await anything in specs without refactoring or adding a just-for-specs promise in ArchiveEditorView. I'll try to take a closer look in the next few days for final review and test things out to reduce the possibility of regressions.

lee-dohm commented 6 years ago

:wave: @Alhadis

We reviewed this in our weekly meeting and I felt a little uncomfortable changing both the code and the tests in the same PR. Can we split the two conversions into separate PRs to ensure that the conversion of the code doesn't break anything? Other than that, I'm 👍 for this change.

Alhadis commented 6 years ago

No problem. 😀 Done.