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

Convert Dock class to Etch #16864

Merged
merged 3 commits into from Mar 30, 2018

Conversation

Projects
None yet
5 participants
@matthewwithanm
Member

matthewwithanm commented Mar 1, 2018

Since #16116 is going to require big changes to how docks are layed out (for which I'll be using etch), I wanted to convert Dock itself. Then I'll update Dock::render() to replace the PaneContainer with a DockGroupContainer (which itself will render DockGroups).

Dock was already written in a super Reacty way so there isn't much being changed here. Basically, render() just returns vdom instead of being a side-effect cesspool.

While working on this, I (re)discovered #16769. In order to be able to verify that I didn't make a mess of things with this PR, I fixed that issue in #16863 and rebased on that. I opened this as a PR against that branch for review purposes, but after that's merged I'll rebase it onto master and switch the base. Please let me know if there was a better way to do it!

@matthewwithanm

This comment has been minimized.

Member

matthewwithanm commented Mar 2, 2018

it's choking on the jsx when building the snapshot here…does this ring any bells for you guys? i'm not familiar with this part of the codebase 😞

@matthewwithanm matthewwithanm changed the base branch from fb-mdt-fix-dock-dragndrop to master Mar 6, 2018

@lee-dohm

This comment has been minimized.

Member

lee-dohm commented Mar 12, 2018

#16936 should be merged at the same time as this.

@daviwil

Looks great to me!

@daviwil daviwil merged commit 31e9dda into master Mar 30, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@daviwil daviwil deleted the fb-mdt-etch-docks branch Mar 30, 2018

@thomasjo

This comment has been minimized.

Member

thomasjo commented Mar 31, 2018

Looks like this broke something. Both @lee-dohm and myself are unable to successfully launch Atom when building from source. Judging from the comments in the code related to snapshotting, I'm guessing something is failing there. Seeing errors like this (in safe mode) upon launching Atom:

Uncaught TypeError: parentElement.insertBefore is not a function
    at updateChildren (/Applications/Atom.app/Contents/Resources/app/node_modules/etch/lib/patch.js:123:27)
    at patch (/Applications/Atom.app/Contents/Resources/app/node_modules/etch/lib/patch.js:18:15)
    at updateSync (/Applications/Atom.app/Contents/Resources/app/node_modules/etch/lib/component-helpers.js:116:26)
    at /Applications/Atom.app/Contents/Resources/app/node_modules/etch/lib/component-helpers.js:72:13
    at ViewRegistry.performDocumentUpdate (/Applications/Atom.app/Contents/Resources/app/src/view-registry.js:238:13)
<embedded>:138043 Uncaught (in promise) TypeError: parentElement.insertBefore is not a function
    at updateChildren (/Applications/Atom.app/Contents/Resources/app/node_modules/etch/lib/patch.js:123:27)
    at patch (/Applications/Atom.app/Contents/Resources/app/node_modules/etch/lib/patch.js:18:15)
    at Object.updateSync (/Applications/Atom.app/Contents/Resources/app/node_modules/etch/lib/component-helpers.js:116:26)
    at Dock.getElement (/Applications/Atom.app/Contents/Resources/app/src/dock.js:88:18)
    at HTMLElement.initialize (/Applications/Atom.app/Contents/Resources/app.asar/src/panel-container-element.js:27:40)
    at PanelContainer.getElement (/Applications/Atom.app/Contents/Resources/app/src/panel-container.js:29:64)
    at HTMLElement.initialize (/Applications/Atom.app/Contents/Resources/app.asar/src/workspace-element.js:117:45)
    at Workspace.getElement (/Applications/Atom.app/Contents/Resources/app/src/workspace.js:254:59)
    at AtomEnvironment.registerDefaultTargetForKeymaps (/Applications/Atom.app/Contents/Resources/app/src/atom-environment.js:339:55)
    at loadStatePromise.loadState.then (/Applications/Atom.app/Contents/Resources/app/src/atom-environment.js:831:18)
    at <anonymous>

@matthewwithanm Any ideas? Only workaround we've got so far is reverting to a commit before this got merged.

@matthewwithanm

This comment has been minimized.

Member

matthewwithanm commented Apr 2, 2018

weird, i'll try to repro.

@thomasjo

This comment has been minimized.

Member

thomasjo commented Apr 2, 2018

@matthewwithanm Let me know if you need anything else, but this is trivially reproducible merely by building from master. We're at least three maintainers so far that are hitting this. Note that we've all setup Atom for full local development, with $ATOM_DEV_RESOURCE_PATH configured, and so on.

My hunch is that this might somehow be related to snapshotting and/or ASAR packaging, since that is the only difference I can think of that sets CI builds apart from local development builds. Not sure if that makes sense though 🤔

@matthewwithanm

This comment has been minimized.

Member

matthewwithanm commented Apr 2, 2018

👍waiting for a rebuild now. i've been running from source and it wasn't reproing.

@matthewwithanm

This comment has been minimized.

Member

matthewwithanm commented Apr 2, 2018

finally finished cleaning and rebuilding and…no repro. any ideas?

@Arcanemagus

This comment has been minimized.

Contributor

Arcanemagus commented Apr 2, 2018

Are you running in --dev? I only see this when running the build as a "normal" Atom (no --dev, no ATOM_DEV_RESOURCE_PATH).

@lee-dohm

This comment has been minimized.

Member

lee-dohm commented Apr 2, 2018

@Arcanemagus No, I wasn't running in Dev Mode when I encountered this error.

@thomasjo

This comment has been minimized.

Member

thomasjo commented Apr 2, 2018

Curiously, running in proper dev mode (--dev) makes the problem go away.
That does point in the direction of this being related to snapshotting/ASAR packaging, right?

@lee-dohm

This comment has been minimized.

Member

lee-dohm commented Apr 2, 2018

Just ran a test using Atom v1.27.0-dev-a066295c7 on Mac OS X 10.13.4 after:

  1. Execute script/clean
  2. Execute git clean -f
  3. Execute script/build --install
  4. Execute atom --dev . (everything works)
  5. Execute atom .

I get the same problems as before.

@daviwil

This comment has been minimized.

Member

daviwil commented Apr 2, 2018

Hey @matthewwithanm, since these changes are causing issues for folks on master, I'm considering reverting those two PRs until we figure out the problem. Do you mind if I revert 31e9dda and a066295 in master so that we can continue the investigation in a separate branch?

@matthewwithanm

This comment has been minimized.

Member

matthewwithanm commented Apr 2, 2018

ah! i was running in dev! thanks for the pointer!

@daviwil yeah, guess we gotta 😞

matthewwithanm added a commit that referenced this pull request Apr 3, 2018

Defer component initialization until element is requested
This fixes the error snapshotting Docks described in #16864.

dolsenj added a commit to dolsenj/atom that referenced this pull request Apr 20, 2018

Added code to perform tab detection when a pane is changed. This is u…
…seful when the tab length of a file differs from the default tab language for a programming language.

Fixed lint errors.

Added spec tests to workspace-spec because there are currently no tests for the tab length detection change. The JavaScript sample files are test inputs for tab length detection.

Only changes the tab length when detected tab length differs from current tab length because this change was reverted.

Reverting back to commit because future commits caused build errors.

⬆️ tree-sitter and language packages

⬆️ text-buffer

Address review feedback

Add exemptLabels to the configuration

Limit the bot to issues only

Capitalize Safe Mode

Revert "⬆️ text-buffer"

This reverts commit 553dec3.

⬆️ line-ending-selector

⬆️ text-buffer

Take 2

Special-casing \r breaks the word regexp

⬆️ find-and-replace

Remove duplicate command bindings for undo and redo

Co-authored-by: Nathan Sobo <nathan@github.com>

Remove JSX

Defer loading of PaneContainerElement to fix snapshotting

⬆️ fuzzy-finder@1.8.1

@smashwilson's updates

Add Teletype highlights from the past week and focus for the coming week

Defer component initialization until element is requested

This fixes the error snapshotting Docks described in atom#16864.

⬆️ tree-view@0.223.0

Revert "⬆️ tree-view@0.223.0"

This reverts commit 139b3c6.

⬆️ one-dark/light-ui@v1.12.0

📝 Document deprecation for `undo` option

Refs atom#16956

⬆️ text-buffer@13.14.0

⬆️ tree-sitter

Weekly focus document template

@smashwilson's update for last week.

More sparse than I like to see 😞

add @annthurium's contributions from last wek

Fix markdown formatting

Fix link

Add Teletype highlights from the past week

Add reactor duty highlights from the past week

Add @daviwil focus for the week

Add Max's focus

Add Xray things

⬆️ autocomplete-plus@2.40.6

⬆️ find-and-replace@0.215.8

⬆️ tree-sitter

⬆️ one-dark/light-ui@v1.12.1

⬆️ find-and-replace

Bump github package version to 0.13.0

bump atom/github to 0.13.2

Fixed snapshotting issue in atom/github#1384

⬆️ spell-check@0.73.2

⬆️ snippets@1.3.2

⬆️ styleguide@v0.49.11

⬆️ text-buffer

⬆️ github

⬆️ spell-check@0.73.3

⬆️ language-python

⬆️ grammar-selector@0.50.1

Separate TextEditor keyboard enablement from readOnly state

Derive the `readonly` attribute from `model.isReadOnly()`

Return the property we're actually setting

Can't use the same name for the property and method

Boolean logic, my old enemy

Increment the version of the readOnly serialization key

👕 restore missing break

Adding additional operating systems to travis.yml because there are issues when multiple operating systems are used.

Changes the tab length only when it is different from the current tab length because it is redundant to set the tab length to itself.

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