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

ES6 broad pass overhaul #4827

Merged
merged 85 commits into from Nov 3, 2017

Conversation

Projects
None yet
5 participants
@dannon
Member

dannon commented Oct 18, 2017

This

  • converts all of our javascript (both amd and commonjs) to es6 modules, and updates require()s left in .mako to use the default exports that were added during translation. we can replace the defaults with named exports as we go, that just wasn't reasonable to do in a single pass
  • replaces callbacks with arrow functions
  • implements string literals
  • minor formatting w/ variable declarations (multi-defines are gone, for readability/explicit handling)
  • Adds a GXY_NOPACK env var you can use when building to beautify instead of uglify the code. This is useful when you're trying to watch the console and variable names/etc are garbled.
  • Adds (cherry-picked @jmchilton's commit from #4832) a make client-format makefile target.

As the very last commit on this branch, I'm going to swap prettier formatting to 120 chars as discussed in #4828 and #4875, to minimize conflict resolution required by anyone merging with this work (so, hopefully one merge instead of having to track through multiple).

The structure view (not show_structure, but structure) needs some help, but that's not exposed anywhere, is also broken in dev, so I'm going to shelve that for right now.

@dannon dannon added the status/WIP label Oct 18, 2017

@dannon dannon referenced this pull request Oct 19, 2017

Closed

Random ES6 Stuff #4832

dannon added some commits Oct 19, 2017

@jmchilton

This comment has been minimized.

Member

jmchilton commented Oct 19, 2017

Any clue how to fix the failing qunit tests? Seeing the same thing after I cleanup imports a different way in my local branch.

@dannon

This comment has been minimized.

Member

dannon commented Oct 19, 2017

@jmchilton Nothing solid, but I think I have an idea as to what it is, but it'll take some tinkering with babel/requirejs.

@dannon

This comment has been minimized.

Member

dannon commented Oct 23, 2017

Closer. Now both bundled and non-bundled es6 code should work in the browser, but there are definitely a few conversion bugs (particular imports vs *, etc).

@jmchilton

This comment has been minimized.

When I was working on this - I dropped the same thing. I looked around the app and I couldn't find any place this was broken after that change.

This comment has been minimized.

Member

dannon replied Oct 24, 2017

Yeah, there's no need for this statement that I could find anywhere; the annoying bit in this file was the _.extend below, tricking babel into dealing with it.

@dannon

This comment has been minimized.

Member

dannon commented Nov 2, 2017

Jupyter will hopefully work now.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Nov 2, 2017

Manually tested on your branch - Jupyter does indeed work for me.

@dannon

This comment has been minimized.

Member

dannon commented Nov 2, 2017

@jmchilton Awesome, thank you. I'm looking for any other weirdness, and plan to take this out of WIP tonight.

@dannon dannon added status/review and removed status/WIP labels Nov 2, 2017

@dannon

This comment has been minimized.

Member

dannon commented Nov 2, 2017

All IES updated. They all do things in slightly different ways; it'd be good to define a single, consistent interface/entrypoint for the launching protocol and update them all. (down the road)

@galaxybot galaxybot added this to the 18.01 milestone Nov 2, 2017

@dannon

This comment has been minimized.

Member

dannon commented Nov 3, 2017

All tests passing, ready for review. If anyone has outstanding javascript work they want to get in today, before this, please let me know and I'll check it out and handle merging it with this; don't want to cause a mess for everyone.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Nov 3, 2017

I got a +1 on this out of band from @guerler who isn't feeling well today but looked it over so I'm going to bite the bullet and merge.

@jmchilton jmchilton merged commit 7196059 into galaxyproject:dev Nov 3, 2017

6 checks passed

api test Build finished. 306 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 162 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 57 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript 10 new alerts and 5 fixed alerts
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@dannon

This comment has been minimized.

Member

dannon commented Nov 3, 2017

Awesome, thanks! If we do find bugs (I'm sure there's something), nothing at this point should be too terrible to fix up.

@bgruening

This comment has been minimized.

Member

bgruening commented Nov 3, 2017

Thanks a lot @dannon! I have so high hopes for the client!

@martenson

This comment has been minimized.

Member

martenson commented Nov 3, 2017

Overall the 120 is huge improvement to me compared to previous prettier settings, readability has been hurt at places but that is just a fallout of 'one size fits all' formatting which is expected.

Thanks for doing this @dannon and let's hope we won't need any more of these all-touching blocking PRs.

@dannon

This comment has been minimized.

Member

dannon commented Nov 3, 2017

@martenson I was just starting one for ES2018, though... /s

@martenson

This comment has been minimized.

Member

martenson commented Nov 3, 2017

🤓

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