JunoLab / atom-julia-client

Juno a good IDE?
http://junolab.org
MIT License
273 stars 72 forks source link

Rollup #737

Open aminya opened 4 years ago

aminya commented 4 years ago

This is another proof of how and why the performance of atom-julia-client is suffering from the old JavaScriptsm and new wrong decisions.

Despite building, it throws all sorts of warnings and errors because of these wrong habits.

Circular dependencies are spread all over the codebase:

(!) Circular dependencies
lib\ui.coffee -> lib\ui\layout.js -> lib\runtime.coffee -> lib\runtime\evaluation.coffee -> lib\ui.coffee
lib\ui.coffee -> lib\ui\layout.js -> lib\runtime.coffee -> lib\runtime\evaluation.coffee -> lib\runtime\workspace.coffee -> lib\ui.coffee
lib\ui.coffee -> lib\ui\layout.js -> lib\runtime.coffee -> lib\runtime\evaluation.coffee -> lib\runtime\workspace.coffee -> C:\Users\yahyaaba\Documents\GitHub\JavaScript\github\julia-client\lib\ui.coffee?commonjs-proxy -> lib\ui.coffee
...and 7 more
created dist in 7.9s

Many unresolved dependencies:

lib/julia-client.coffee β†’ dist...
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
child_process (imported by lib\misc\paths.js, lib\connection\terminal.coffee, child_process?commonjs-external)
net (imported by lib\connection\terminal.coffee, net?commonjs-external, lib\connection\process\basic.js, lib\connection\process\remote.js, lib\connection\process\tcp.coffee)
vm (imported by lib\runtime\frontend.coffee, vm?commonjs-external)
shell (imported by lib\package\commands.coffee, shell?commonjs-external)
remote (imported by lib\ui\notifications.coffee, remote?commonjs-external)

Many other dynamic requires that I tried to solve using plugins.

My suggestion for this repository:

pfitzseb commented 4 years ago

Can you explain what rollup is and why we should use it?

Anyways, I agree with most of your points. We definitely should restructure code at the same time as translating it form CoffeeScript to JS, which is precisely why automating that task is only somewhat helpful.

Circular dependencies are spread all over the codebase:

Yeah, that's pretty bad...

Many unresolved dependencies:

Eh, those are whatever -- in theory we shouldn't rely on those being in the global context (just like the atom global, I suppose), but in practise that really doesn't matter much.

Use named exports/imports Stop using dynamic require.

πŸ‘

Don't try to do everything in one package. Use services from other packages like linter, atom-ide packages, etc. Modularity is the best development approach.

Eh, not so sure about that. Most Atom packages aren't that great to interface with (and/or provide an awful UX). atom-linter is probably the exception there, and we should use it once we actually have a linter (and for the Traceur integration, I suppose).

deferring execution should be the last thing to try. The program should already run fast synchronized and then could benefit from asynchronous execution.

Hm, not sure what you mean here. I think we should make sure to (re-) design everything with lazy-loading and as much asynchronicity in mind -- it's pretty hard to shoehorn that in afterwards without making a mess of everything.

and lastly, be open to suggestions. People want to help.

πŸ‘ But also appreciate that we don't have an unlimited amount of time to work on this (and even reviewing PRs takes a lot of time when they touch huge parts of the codebase). I also think performance isn't the biggest problem Juno has right now.

aminya commented 4 years ago

Can you explain what rollup is and why we should use it?

It is a bundler similar to Webpack, but with better plugins. It reduced the load time (even now that the code has many issues) to half of its time. So once the Junos requires are converted to import,export, you can get a huge bump in the performance.

2020-05-01 05_44_00-Settings β€” C__Users_yahyaaba_Documents_GitHub_JavaScript_github_julia-client β€” A

2020-05-01 05_44_37-Settings β€” C__Users_yahyaaba_Documents_GitHub_JavaScript_github_julia-client β€” A

Don't try to do everything in one package. Use services from other packages like linter, atom-ide packages, etc. Modularity is the best development approach.

Eh, not so sure about that. Most Atom packages aren't that great to interface with (and/or provide an awful UX). atom-linter is probably the exception there, and we should use it once we actually have a linter (and for the Traceur integration, I suppose).

atom-ide-community packages are very well. linter is very good too. If there is something you don't like about them, you should improve those packages instead of writing everything from scratch here.

deferring execution should be the last thing to try. The program should already run fast synchronized and then could benefit from asynchronous execution.

Hm, not sure what you mean here. I think we should make sure to (re-) design everything with lazy-loading and as much asynchronicity in mind -- it's pretty hard to shoehorn that in afterwards without making a mess of everything.

Here I meant relying on deferring execution isn't good when the codebase already has other issues. It should be tried after.

and lastly, be open to suggestions. People want to help.

πŸ‘ But also appreciate that we don't have an unlimited amount of time to work on this (and even reviewing PRs takes a lot of time when they touch huge parts of the codebase).

That is why you opened the source of the packages. People will put time on helping the package improve.

I also think performance isn't the biggest problem Juno has right now. Writing good code is always important, even if you don't care about performance. The things I mentioned are not that complicated, most of them are a matter of just replacing a few lines of code.

Regarding the Atom's performance, people complain about it all over the internet. Many (including the actual Atom developers) have already switched to other platforms! We should all write performant code, especially when we gain a lot with very minor adjustments (like import and require).