-
Notifications
You must be signed in to change notification settings - Fork 366
Move off of dryice and to webpack for builds #205
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
Conversation
|
Looks like we will need to drop node 0.8.X for webpack which is fine by me. Its years old. |
We were pretty much the last dryice user, and it isn't worth being on such a minority build tool when we don't really want to maintain it ourselves. Additionally, this lets us stop using amdefine and the AMD format, which was an alright choice in 2011 but has increasinly shown its age. Regular Common JS modules are used now, which should pave the way for better tree shaking and unused submodule removal tooling. Note that in order to avoid ruining `git blame`, this leaves each module indented by two spaces as it was from the AMD `define` function, and wraps a block around them so that editors don't freak out too much.
383dc64 to
44f9b7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're building each test file with webpack, which includes a require of the sourcemap lib. Won't that generate test files that each bundle in the entire sourcemap lib?
If you're generating files that are meant to run on our try server, you probably want to add an alias that resolves the sourcemap library to whatever the path is in firefox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I just looked at the test files and they require specific individual modules from the project. So you probably intended to just bundle them all together. Makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah some duplication, but only in tests. The build process is just so much more sane than it used to be, that I think this is worth it.
|
Personally I'd say to just re-indent everything. You can pass My main concern is building the test files. r+ if you make sure it's not actually bundling in the sourcemap library for all of them, which doesn't seem desirable |
|
I looked at it further and I understand what you're doing now. r+ |
|
Thanks for the review! I'd rather not re-indent, even with |
Move off of dryice and to webpack for builds
|
@fitzgen Can you publish this please? |
|
I will later today, after @jlongster reviews the follow ups in #206. |
|
@fitzgen You can publish it (I only had 1 or 2 nits I think) but followup with looking into TravisCI for generating builds if you want later, doesn't need to block it. |
|
Its been published. |
We were pretty much the last dryice user, and it isn't worth being on such a
minority build tool when we don't really want to maintain it
ourselves. Additionally, this lets us stop using amdefine and the AMD format,
which was an alright choice in 2011 but has increasingly shown its age. Regular
Common JS modules are used now, which should pave the way for better tree
shaking and unused submodule removal tooling. Note that in order to avoid
ruining
git blame, this leaves each module indented by two spaces as it wasfrom the AMD
definefunction, and wraps a block around them so that editorsdon't freak out too much.
r? @jlongster
cc @sokra