Skip to content
This repository was archived by the owner on Oct 2, 2019. It is now read-only.

refactor(build): replace Gulp build with Webpack #1973

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jrbotros
Copy link

@jrbotros jrbotros commented Apr 14, 2017

Address part of #1096 and #1966.

This is pretty much all working, but I'm getting a couple frustrating errors in the tests. It looks like there's no .ui-select-match rendered by the time this transcludeFn runs for the selectize theme (getting the same error as in #1444).

It looks like in selectize/match-multiple.tpl.html the ui-select-match class only exists on ng-repeat elements, so maybe those elements haven't been loaded yet? I modified the directives to use the template HTML directly, which I'm assuming subtly changed the timing of template loading and broke the sort-of-weird transclusion logic. Any help/ideas here would be much appreciated!

Also, what's the easiest way to actually see the code in use? I didn't see any examples that read in from the dev code directly, but I could be just missing something obvious! The other tests pass, but want to make sure everything looks shipshape.

@ProLoser @Jefiozie I know you both had interest in getting things cleaned up, so let me know what you think. If this looks good, I'd love to do a broader ES6 rewrite and standardize code style.

@ProLoser
Copy link
Member

Can you fix yarn and travis so that we can see the errors you're getting?

@jrbotros
Copy link
Author

My bad, bumped the node by a minor version which looks like it fixed things.

@ProLoser
Copy link
Member

Sorry I'm too far removed from angular and this code to be of any help any longer. I was looking through it but I can't recall at all what's going on.

@jrbotros
Copy link
Author

Gotcha, do you know anyone who would be the go-to person? I'd be down to rewrite some of the transclusion code but I'd love to get this build stuff working without changing any underlying logic.

@Jefiozie any ideas on your end?

@ProLoser
Copy link
Member

ProLoser commented Apr 14, 2017

You can try brute force debugging. Try changing the logic back to using templateUrl (temporarily) and see if that fixes it. Try to eliminate other factors by hard coding and reverting things in small pieces until you can identify the problem area. Then once that problem area begins working, try rolling back your modifications to the other code and see if it remains working until you eventually are left with the only part of code that breaks the build temporarily hardcoded hopefully making it easier to fix.

@Jefiozie
Copy link
Contributor

Hi,

First off all thanks for all the effort you already put in this PR.

As I described in #1966 I feel the need to go this way but at the moment I think we first focus us on getting the code cleaned. One of the main reasons there are this many bugs is that everything it put in the one or two files that should have been put in separate directives.

One of the things we will see during the refactoring is that we will have code that is being used ( one of the points you pointed out).

Do you agree on this? Or would you first convert to es6 before cleaning it up?

@ProLoser
Copy link
Member

Based on my own experience and Ember's findings: small transitions are best, don't focus on waiting for major factors if you can. Also, don't focus on backwards compatibility as it can be very limiting. You can emulate angular by using frequent major releases, deprecating and then removing old stuff, and just trying to iterate quickly.

@jrbotros
Copy link
Author

Converting to ES6 should be a pretty painless process, I'm mainly just planning on making sure the repo conforms to a standard style (airbnb is my preference, but let me know if you feel differently) without doing any logic refactors. Once webpack is set up, we can do this in as small chunks as we want, probably something like one file at a time. And then I think it will be much easier to see how all the parts fit together. Also, we'll have access to all the luxuries of ES6 (the module system in particular), which I think will be crucial in a broader cleanup. @Jefiozie does that sound reasonable?

I'll try to fix the transclusion bug in the next few days—will keep you updated!

@jrbotros
Copy link
Author

I fixed the transclude bug by adding the ui-select-match-wrapper class to all the match templates. Not very elegant, but a good hack to get this in, and later we can explore transclusion slots in Angular 1.5 or a third-party solution if we don't want to upgrade. I think this is ready and the demos in my local docs are working for me, but would love a second look!

@Jefiozie did you have thoughts on the refactor strategy I proposed? I'd be happy to finish file-wise ES6ifying by the end of the weekend!

@Jefiozie
Copy link
Contributor

Hi @jrbotros , I will try and have a look today at your PR and make a comment on the way forward.
Sorry for the delay busy times 😄

@Jefiozie
Copy link
Contributor

So I had a look at your repo and I think we need to merge it (in the end).
Can we also merge the test suite to webpack? (as in can we run the tests before building the bundle?)

Probably have some questions later but for now I think we need to get this finished and merged.

@jrbotros
Copy link
Author

Awesome, sounds like a plan!

We can't run the tests before building the bundle since the code needs to be transpiled from ES6 first. Right now karma is set up to use webpack, so karma start will internally watch and bundle before running the tests and gulp test will work the same as before. (Karma will also transpile the tests, so we can use ES6 there, too.)

@Jefiozie Jefiozie self-requested a review April 23, 2017 19:36
Copy link
Contributor

@Jefiozie Jefiozie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clean up the dist folder?

@jrbotros
Copy link
Author

jrbotros commented Jun 7, 2017

What specifically do you want cleaned up? Since we're now using webpack it's a good idea to include all the sourcemaps, and I've included the minified build as well for users who want that optimization.

@jrbotros
Copy link
Author

jrbotros commented Jun 7, 2017

Actually forgot—I made sure to build exactly the same files in dist that are currently there!

@jrbotros
Copy link
Author

@Jefiozie Any update here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants