Skip to content

Conversation

@fitzgen
Copy link
Contributor

@fitzgen fitzgen commented Aug 28, 2015

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 was
from the AMD define function, and wraps a block around them so that editors
don't freak out too much.

r? @jlongster

cc @sokra

@fitzgen
Copy link
Contributor Author

fitzgen commented Aug 28, 2015

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.
@fitzgen fitzgen force-pushed the use-webpack-instead-of-dryice branch from 383dc64 to 44f9b7d Compare August 28, 2015 20:17
@fitzgen fitzgen mentioned this pull request Aug 29, 2015

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.

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.

Copy link
Contributor Author

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.

@jlongster
Copy link

Personally I'd say to just re-indent everything. You can pass -w to git blame to just ignore changes that are just whitespace, which you probably want to make the default anyway.

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

@jlongster
Copy link

I looked at it further and I understand what you're doing now. r+

@fitzgen
Copy link
Contributor Author

fitzgen commented Aug 31, 2015

Thanks for the review!

I'd rather not re-indent, even with git blame -w for the sake of ease of merging outstanding PRs and branches in the future.

fitzgen added a commit that referenced this pull request Aug 31, 2015
Move off of dryice and to webpack for builds
@fitzgen fitzgen merged commit f077b86 into mozilla:master Aug 31, 2015
@zertosh
Copy link
Contributor

zertosh commented Sep 1, 2015

@fitzgen Can you publish this please?

@fitzgen
Copy link
Contributor Author

fitzgen commented Sep 1, 2015

I will later today, after @jlongster reviews the follow ups in #206.

@jlongster
Copy link

@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.

@fitzgen
Copy link
Contributor Author

fitzgen commented Sep 10, 2015

Its been published.

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

Labels

None yet

3 participants