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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove atom-space-pen-views #913

Merged
merged 45 commits into from Mar 3, 2017

Conversation

Projects
None yet
3 participants
@as-cii
Member

as-cii commented Feb 16, 2017

This pull request replaces atom-space-pen-views with etch and, where appropriate, also manual DOM manipulation. Feature-wise nothing should change, but this will help remove jQuery from Atom.

/cc: @ungb for 馃憖

as-cii added some commits Feb 6, 2017

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Feb 17, 2017

Contributor

I'm getting the following error installing language-markdown with this enabled. Works fine when I apm unlink setting-view from your branch.

Atom: 1.14.2 x64
Electron: 1.3.13
OS: Mac OS X 10.12.2
Thrown From: settings-view package 0.247.0

Stack Trace

Uncaught NotFoundError: Failed to execute 'replaceChild' on 'Node': The node to be replaced is not a child of this node.

At /Users/bungdonuts/github/packages/jqueryTest/settings-view/lib/package-detail-view.js:107

Error: Failed to execute 'replaceChild' on 'Node': The node to be replaced is not a child of this node.
    at Error (native)
    at PackageDetailView.completeInitialzation (/packages/jqueryTest/settings-view/lib/package-detail-view.js:107:35)
    at PackageDetailView.loadPackage (/packages/jqueryTest/settings-view/lib/package-detail-view.js:124:12)
    at /packages/jqueryTest/settings-view/lib/package-detail-view.js:382:14
    at Function.module.exports.Emitter.simpleDispatch (/app.asar/node_modules/event-kit/lib/emitter.js:25:14)
    at Emitter.module.exports.Emitter.emit (/app.asar/node_modules/event-kit/lib/emitter.js:129:28)
    at PackageManager.module.exports.PackageManager.emitPackageEvent (/Users/bungdonuts/github/atom/out/app/node_modules/settings-view/lib/package-manager.coffee:449:14)
    at /Users/bungdonuts/github/atom/out/app/node_modules/settings-view/lib/package-manager.coffee:342:10
    at exit (/Users/bungdonuts/github/atom/out/app/node_modules/settings-view/lib/package-manager.coffee:62:7)
    at triggerExitCallback (/app.asar/src/buffered-process.js:303:11)
    at /app.asar/src/buffered-process.js:316:11
    at /app.asar/src/buffered-process.js:185:9)
    at emitOne (events.js:101:20)
    at Socket.emit (events.js:188:7)
    at Pipe._handle.close [as _onclose] (net.js:493:12)

Commands

Non-Core Packages

git-plus 7.2.2 
github 0.0.0 
language-markdown 0.19.1 
seti-syntax 1.1.2 
seti-ui 1.6.1 
slack-ui 0.8.0 
snippet-creator 0.0.0 
spotify-ui 1.2.2 
Sublime-Style-Column-Selection 1.7.3 
sublimify 0.5.0 
terminal-plus 0.14.5 
Contributor

ungb commented Feb 17, 2017

I'm getting the following error installing language-markdown with this enabled. Works fine when I apm unlink setting-view from your branch.

Atom: 1.14.2 x64
Electron: 1.3.13
OS: Mac OS X 10.12.2
Thrown From: settings-view package 0.247.0

Stack Trace

Uncaught NotFoundError: Failed to execute 'replaceChild' on 'Node': The node to be replaced is not a child of this node.

At /Users/bungdonuts/github/packages/jqueryTest/settings-view/lib/package-detail-view.js:107

Error: Failed to execute 'replaceChild' on 'Node': The node to be replaced is not a child of this node.
    at Error (native)
    at PackageDetailView.completeInitialzation (/packages/jqueryTest/settings-view/lib/package-detail-view.js:107:35)
    at PackageDetailView.loadPackage (/packages/jqueryTest/settings-view/lib/package-detail-view.js:124:12)
    at /packages/jqueryTest/settings-view/lib/package-detail-view.js:382:14
    at Function.module.exports.Emitter.simpleDispatch (/app.asar/node_modules/event-kit/lib/emitter.js:25:14)
    at Emitter.module.exports.Emitter.emit (/app.asar/node_modules/event-kit/lib/emitter.js:129:28)
    at PackageManager.module.exports.PackageManager.emitPackageEvent (/Users/bungdonuts/github/atom/out/app/node_modules/settings-view/lib/package-manager.coffee:449:14)
    at /Users/bungdonuts/github/atom/out/app/node_modules/settings-view/lib/package-manager.coffee:342:10
    at exit (/Users/bungdonuts/github/atom/out/app/node_modules/settings-view/lib/package-manager.coffee:62:7)
    at triggerExitCallback (/app.asar/src/buffered-process.js:303:11)
    at /app.asar/src/buffered-process.js:316:11
    at /app.asar/src/buffered-process.js:185:9)
    at emitOne (events.js:101:20)
    at Socket.emit (events.js:188:7)
    at Pipe._handle.close [as _onclose] (net.js:493:12)

Commands

Non-Core Packages

git-plus 7.2.2 
github 0.0.0 
language-markdown 0.19.1 
seti-syntax 1.1.2 
seti-ui 1.6.1 
slack-ui 0.8.0 
snippet-creator 0.0.0 
spotify-ui 1.2.2 
Sublime-Style-Column-Selection 1.7.3 
sublimify 0.5.0 
terminal-plus 0.14.5 
@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Feb 19, 2017

Member

Thanks for the report @ungb. I was unable to reproduce locally, but I analyzed the code and tried to find possible ways in which it could have thrown that exception. I discovered a potential code path that could be causing what you experienced and made an attempt at fixing it in d3dba5e.

Please, let me know if you are still able to reproduce. If yes, we could maybe 馃崘 on Tuesday in order to debug the issue on your machine and garner data to fix it.

Member

as-cii commented Feb 19, 2017

Thanks for the report @ungb. I was unable to reproduce locally, but I analyzed the code and tried to find possible ways in which it could have thrown that exception. I discovered a potential code path that could be causing what you experienced and made an attempt at fixing it in d3dba5e.

Please, let me know if you are still able to reproduce. If yes, we could maybe 馃崘 on Tuesday in order to debug the issue on your machine and garner data to fix it.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Feb 20, 2017

Member

The Open Config Folder button does nothing when I have this PR linked. In Atom 1.15.0-beta3 it works.

Member

Ben3eeE commented Feb 20, 2017

The Open Config Folder button does nothing when I have this PR linked. In Atom 1.15.0-beta3 it works.

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Feb 20, 2017

Member

Thank you for the heads-up, @Ben3eeE! It should be fixed by 75e8896.

Member

as-cii commented Feb 20, 2017

Thank you for the heads-up, @Ben3eeE! It should be fixed by 75e8896.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Feb 21, 2017

Contributor

The issue is now fixed for me @as-cii after your latest change regarding the issue I was having installing packages.

Under Available updates I'm seeing:
Before your Change:
image

After your change:

image

I'm not sure why I'm seeing undefined exactly, but it's not related to your change. The style changed though.

Contributor

ungb commented Feb 21, 2017

The issue is now fixed for me @as-cii after your latest change regarding the issue I was having installing packages.

Under Available updates I'm seeing:
Before your Change:
image

After your change:

image

I'm not sure why I'm seeing undefined exactly, but it's not related to your change. The style changed though.

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Feb 22, 2017

Member

I'm not sure why I'm seeing undefined exactly, but it's not related to your change. The style changed though.

Nice find! I have fixed it in 6631f9c.

Member

as-cii commented Feb 22, 2017

I'm not sure why I'm seeing undefined exactly, but it's not related to your change. The style changed though.

Nice find! I have fixed it in 6631f9c.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Mar 2, 2017

Contributor

Looks good to me, but looks like there's a conflict. Will look at this again after the merge conflict is resolve.

One little issue I found is if you open the list view in packages/theme/install and click on the author name, before your change it open the author page, Now it just goes into the package. If you click on the author name there, it goes to the author page.

image

Contributor

ungb commented Mar 2, 2017

Looks good to me, but looks like there's a conflict. Will look at this again after the merge conflict is resolve.

One little issue I found is if you open the list view in packages/theme/install and click on the author name, before your change it open the author page, Now it just goes into the package. If you click on the author name there, it goes to the author page.

image

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Mar 2, 2017

Member

Got this now. I am on 75e8896 which is after the supposed fix but not latest commit.

I'll pull latest and try to reproduce it.

C:\Users\lineri\Desktop\issues\atom master\settings-view\lib\package-detail-view.js:108
Hide Stack Trace
Error: Failed to execute 'replaceChild' on 'Node': The node to be replaced is not a child of this node.
    at Error (native)
    at PackageDetailView.completeInitialization (package-detail-view.js:108:35)
    at PackageDetailView.loadPackage (package-detail-view.js:125:12)
    at disposables.add.packageManager.on (package-detail-view.js:383:14)
    at Function.module.exports.Emitter.simpleDispatch (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-56f55e9\resources\app.asar\node_modules\event-kit\lib\emitter.js:25:14)
    at Emitter.module.exports.Emitter.emit (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-56f55e9\resources\app.asar\node_modules\event-kit\lib\emitter.js:129:28)
    at PackageManager.module.exports.PackageManager.emitPackageEvent (file:///C:/Users/lineri/Desktop/issues/atom master/settings-view/lib/package-manager.coffee:449:14)
    at file:///C:/Users/lineri/Desktop/issues/atom master/settings-view/lib/package-manager.coffee:342:10
    at exit (file:///C:/Users/lineri/Desktop/issues/atom master/settings-view/lib/package-manager.coffee:62:7)
    at triggerExitCallback (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-56f55e9\resources\app.asar\src\buffered-process.js:265:9)
    at ChildProcess.process.on.code (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-56f55e9\resources\app.asar\src\buffered-process.js:295:9)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:215:12)
Member

Ben3eeE commented Mar 2, 2017

Got this now. I am on 75e8896 which is after the supposed fix but not latest commit.

I'll pull latest and try to reproduce it.

C:\Users\lineri\Desktop\issues\atom master\settings-view\lib\package-detail-view.js:108
Hide Stack Trace
Error: Failed to execute 'replaceChild' on 'Node': The node to be replaced is not a child of this node.
    at Error (native)
    at PackageDetailView.completeInitialization (package-detail-view.js:108:35)
    at PackageDetailView.loadPackage (package-detail-view.js:125:12)
    at disposables.add.packageManager.on (package-detail-view.js:383:14)
    at Function.module.exports.Emitter.simpleDispatch (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-56f55e9\resources\app.asar\node_modules\event-kit\lib\emitter.js:25:14)
    at Emitter.module.exports.Emitter.emit (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-56f55e9\resources\app.asar\node_modules\event-kit\lib\emitter.js:129:28)
    at PackageManager.module.exports.PackageManager.emitPackageEvent (file:///C:/Users/lineri/Desktop/issues/atom master/settings-view/lib/package-manager.coffee:449:14)
    at file:///C:/Users/lineri/Desktop/issues/atom master/settings-view/lib/package-manager.coffee:342:10
    at exit (file:///C:/Users/lineri/Desktop/issues/atom master/settings-view/lib/package-manager.coffee:62:7)
    at triggerExitCallback (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-56f55e9\resources\app.asar\src\buffered-process.js:265:9)
    at ChildProcess.process.on.code (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-56f55e9\resources\app.asar\src\buffered-process.js:295:9)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:215:12)
@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Mar 2, 2017

Contributor

@Ben3eeE what were you doing when you saw this? I couldn't repro it after the fix, I'll try again now.

Contributor

ungb commented Mar 2, 2017

@Ben3eeE what were you doing when you saw this? I couldn't repro it after the fix, I'll try again now.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Mar 2, 2017

Member

@ungb @as-cii This is what I did to reproduce the issue.
jquery settings view install error

Member

Ben3eeE commented Mar 2, 2017

@ungb @as-cii This is what I did to reproduce the issue.
jquery settings view install error

as-cii added some commits Mar 3, 2017

Merge branch 'master' into as-remove-jquery
# Conflicts:
#	lib/settings-view.coffee
#	lib/updates-panel.coffee
#	spec/settings-view-spec.coffee
#	spec/themes-panel-spec.coffee
@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Mar 3, 2017

Member

Thank you so much for reproducing this, @Ben3eeE! I have fixed it in 480ff47: I'll go ahead and merge this pull request as soon as we get a green build.

Member

as-cii commented Mar 3, 2017

Thank you so much for reproducing this, @Ben3eeE! I have fixed it in 480ff47: I'll go ahead and merge this pull request as soon as we get a green build.

@as-cii as-cii merged commit 3d76197 into master Mar 3, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@as-cii as-cii deleted the as-remove-jquery branch Mar 3, 2017

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