Skip to content
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

Run through Ember Build Pipeline #2687

Closed
wants to merge 1 commit into from

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle
Copy link
Contributor Author

This is currently a WIP in concert with emberjs/emberjs-build#39

@seanpdoyle
Copy link
Contributor Author

Current blocker:

Uncaught Error: Could not find module ember-inflector/system/string

Ember Build can't accept additional ES6 modules (like those found here https://github.com/emberjs/emberjs-build/blob/6b68577851a92ab989754f93ad832c14b8a54cc9/lib/get-vendored-packages.js#L13-L17)

@bmac
Copy link
Member

bmac commented Jan 16, 2015

Hey @seanpdoyle whats the current status on this pr. Are you still blocked on importing ember-inflector?

@seanpdoyle
Copy link
Contributor Author

@bmac yes, sort of. @twokul has recently refactored some of the emberjs-build project, so I have to catch up to the work he's done there. I might have some new issues stemming from his changes, but my biggest problem has been figuring out how to properly vendor / import / concat ember-data, activemodel_adapter, and ember-inflector

@rwjblue
Copy link
Member

rwjblue commented Jan 16, 2015

@seanpdoyle - I'd be happy to do a hangout early next week to make a plan on this.

@seanpdoyle
Copy link
Contributor Author

@rwjblue 👍 feel free to connect through email, hangouts, screenhero (all seandoyle at thoughtbot.com)

@seanpdoyle
Copy link
Contributor Author

ping @rwjblue: do you have availability this week?

@seanpdoyle
Copy link
Contributor Author

I'm running against https://github.com/seanpdoyle/emberjs-build/compare/wip-get-vendored-packages

and when I visit http://localhost:4200/tests/index.html and put a breakpoint on

 if (!registry[name]) {
  throw new Error("Could not find module " + name);
}

ember-inflector doesn't have a top level module. ember-inflector/main is the closest thing:

screen shot 2015-01-26 at 3 49 07 pm

cc: @twokul @rwjblue -- Any ideas?

@rwjblue
Copy link
Member

rwjblue commented Jan 26, 2015

@seanpdoyle - ember-inflector/lib/main.js should be moved to ember-inflector.js which would make it ember-inflector module. In other words packages/<some-name>/lib/main.js is <some-name> module.

@fivetanley
Copy link
Member

i will gladly merge any commits to ember-inflector for it.

as for activemodel-adapter we have made plans to extract it into its own repository. We can start work on that and transfer it to the emberjs org later if that's more convenient.

@seanpdoyle
Copy link
Contributor Author

@rwjblue do you mean in a build step, or do you mean in the actual source code at stefanpenner/ember-inflector?

@rwjblue
Copy link
Member

rwjblue commented Jan 26, 2015

@seanpdoyle - Build step. https://github.com/emberjs/emberjs-build/blob/master/lib/htmlbars-package.js does roughly what you want (but you need to supply a libPath like in emberjs/emberjs-build#63).

vendoredPackages: {
'loader': vendoredPackage('loader'),
'ember-inflector': vendoredES6Package('ember-inflector', {
libPath: 'bower_components/ember-inflector/packages/ember-inflector/lib',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue I thought this did what you're talking about in #2687 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Note that https://github.com/emberjs/emberjs-build/blob/master/lib/es6-vendored-package.js does not move main.js to <package-name.js> like the section I linked. But we should probably consolidate the htmlbarsPackage and vendoredES6Package...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue does this section https://github.com/emberjs/emberjs-build/blob/master/lib/htmlbars-package.js#L20-L28 do the renaming through the includes option for Funnel?

Copy link
Member

Choose a reason for hiding this comment

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

Crap, I'm making this as clear as mud. This is what you need to move main up to the main package name: https://github.com/emberjs/emberjs-build/blob/master/lib/vendored-package.js#L28-L37

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌 that seems to have worked, but now
screen shot 2015-01-26 at 5 20 45 pm

It can't find <script src="qunit_configuration"> and 0 tests are running.

I might just hold off until we have our hangout. I don't think I fully understand all the moving parts

Copy link
Member

Choose a reason for hiding this comment

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

Basically this: screenshot

@seanpdoyle seanpdoyle force-pushed the use-emberjs-build branch 2 times, most recently from 5e0122a to 5500641 Compare January 28, 2015 21:13
"es6-module-transpiler": "^0.9.5",
"es6-module-transpiler-amd-formatter": "^0.2.4",
"es6-module-transpiler-package-resolver": "^1.0.1",
"emberjs-build": "file:../emberjs-build",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue this needs to be replaced, obviously, but this PR depends on emberjs/emberjs-build#65

@seanpdoyle seanpdoyle force-pushed the use-emberjs-build branch 2 times, most recently from 408dee3 to 9665fdb Compare January 28, 2015 21:30
@seanpdoyle
Copy link
Contributor Author

I'll try and tidy up the git history to tell a clearer story.

@seanpdoyle
Copy link
Contributor Author

Heres the diff of ember build --environment production, with master on the left and this branch on the right

https://www.diffchecker.com/2nix5m8n

@twokul
Copy link
Member

twokul commented Jan 28, 2015

@seanpdoyle no source maps?

@seanpdoyle
Copy link
Contributor Author

@rwjblue how does ember proper export source maps?

@rwjblue
Copy link
Member

rwjblue commented Jan 29, 2015

@seanpdoyle / @twokul - emberjs-build does not currently generate sourcemaps. I am nearly finished with the migration to esperanto as the module transpiler, and we should be able to support sourcemaps for both projects once that effort is finished.

@seanpdoyle
Copy link
Contributor Author

@rwjblue ember build is only outputting the ember-data.debug.js. It's skipping the .prod and .min versions of both ember-data and ember-data-tests.

I'm using this version of ember-build.

Any ideas?

});
testFunctions.push(function() {
return run('dist=prod&emberchannel=' + emberChannel);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue I'm trying to setup npm test to run against debug, prod and min versions of the code.

ember build is only outputting the ember-data.debug.js. It's skipping the .prod and .min versions of both ember-data and ember-data-tests.

I'm using this version of ember-build.

They exist when I run ember build --environment production, which is fine for CI where broccoli's environment is production, but locally only the first test run will work.

ember proper doesn't seem to be doing anything differently from the ember-build perspective.

Any ideas?

@seanpdoyle
Copy link
Contributor Author

@rwjblue @twokul I think this is ready for a final look. Once I get the ok I'll squash into more meaningful commits

@rwjblue
Copy link
Member

rwjblue commented Jan 29, 2015

ember build is only outputting the ember-data.debug.js. It's skipping the .prod and .min versions of both ember-data and ember-data-tests.

@seanpdoyle - It only generates prod and min output when running against the production environment (this ensures that dev builds are MUCH faster). So ember build --environment=production should do the trick here.

@seanpdoyle
Copy link
Contributor Author

Looks like this is failing against canary because of too many deprecation warnings logged.

https://travis-ci.org/emberjs/data/jobs/48790205

Should this PR address that?

@seanpdoyle
Copy link
Contributor Author

@rwjblue also, I can't believe I've never asked this, but is the monkey patching a test-only fix?

@seanpdoyle
Copy link
Contributor Author

@rwjblue how would I go about changing emberjs-build to include bower_components so that this line

<script src="/bower_components/qunit/qunit/qunit.js"></script>

loads qunit.js et al.?

I tried adding /bower_components/ to https://github.com/emberjs/emberjs-build/blob/e13695255dcc33ecabd509bd5d719a54e961335d/lib/config/test-config-tree.js#L11 but it wasn't included

@rwjblue
Copy link
Member

rwjblue commented Mar 29, 2015

@seanpdoyle - It is available at /qunit/qunit.js by default, you should be able to update that script tag to something like:

<script src="../qunit/qunit.js"></script>

@seanpdoyle
Copy link
Contributor Author

@rwjblue this is green depending on emberjs/emberjs-build#89 being merged

@rwjblue
Copy link
Member

rwjblue commented Mar 29, 2015

@seanpdoyle - I do not believe that an update to emberjs-build is needed (at least, not to get qunit to load). Did you see my comment above?

@seanpdoyle
Copy link
Contributor Author

@rwjblue I did, but ember data needs (wants?) the ember bower component.

We could probably get by without it, but that means we'd pull from builds.emberjs.com instead of the local file system

@rwjblue
Copy link
Member

rwjblue commented Mar 29, 2015

that means we'd pull from builds.emberjs.com instead of the local file system

Which unfortunately is what is happening today.

I agree that changes are needed, sorry for the quick reply without digging into the reasoning more. I commented on the emberjs-build pull request with some suggestions.

@seanpdoyle
Copy link
Contributor Author

@rwjblue this is green for release, but for beta and canary, deprecation warnings are flooding travis and failing an otherwise green build.

https://travis-ci.org/emberjs/data/jobs/56345865#L229

DEPRECATION: Using the same function as getter and setter is deprecated. See http://emberjs.com/deprecations/v1.x/#toc_deprecate-using-the-same-function-as-getter-and-setter-in-computed-properties for more details.

This change is introduced in 1.12, so I can't introduce the fix without breaking release.

Is there a way to silence deprecations?

@seanpdoyle
Copy link
Contributor Author

@rwjblue @fivetanley @bmac ping

@rwjblue
Copy link
Member

rwjblue commented Apr 2, 2015

@seanpdoyle the deprecations are fixed on master in #2961 but that is not what was causing it to fail. The actual issue which was causing the failure was fixed in #2959.

Can you rebase to get these updates?

@seanpdoyle
Copy link
Contributor Author

screen shot 2015-04-02 at 9 40 45 am

@rwjblue what a time to be alive

@seanpdoyle
Copy link
Contributor Author

Looks like the windows build is failing because it's using node 0.12.x (https://ci.appveyor.com/project/embercli/data/build/724#L325) instead of 0.10.x

@rwjblue
Copy link
Member

rwjblue commented Apr 2, 2015

Confirm that issue is being tracked in emberjs/emberjs-build#87, but I am unsure if @stefanpenner has had time to work on it more.

* Share build process found in [emberjs/emberjs-build][1]
* Easiest approach was to copy paste from [emberjs/ember.js][2]
* In the long term, we should move common build tasks / setup to [emberjs/emberjs-build][1]
* Vendor `ember-inflector` ES6 package

Silence Deprecations

* Use `container._registry.has` instead of `container.has`
* Use `env.registry` instead of `store.container`

[1]: https://github.com/emberjs/emberjs-build
[2]: https://github.com/emberjs/ember.js

* Resolve JSCS infractions
* Move `package-manager-files` for `emberjs-build`
* Pass Bower Funnels into emberjs-build
* Run appveyor against node `0.10`
@seanpdoyle
Copy link
Contributor Author

note to self:

squashed and force pushed over

(function() {
+ window.EmberDev = window.EmberDev || {};
+ EmberDev.runningProdBuild = !!QUnit.urlParams.prod;
+ // hack qunit to not suck for Ember objects
+ var originalTypeof = QUnit.jsDump.typeOf;
+ QUnit.jsDump.typeOf = function(obj) {
+ if (Ember && Ember.Object && Ember.Object.detectInstance(obj)) {
+ return "emberObject";
+ }
+ return originalTypeof.call(this, obj);
+ };
+ // raises is deprecated, but we likely want to keep it around for our es3
+ // test runs.
+ QUnit.constructor.prototype.raises = QUnit['throws'];
+ window.raises = QUnit['throws'];
+ QUnit.jsDump.parsers.emberObject = function(obj) {
+ return obj.toString();
+ };
+ var EmberDevTestHelperAssert = window.Ember.__loader.require('ember-dev/test-helper/index')['default'];
+ var setupQUnit = window.Ember.__loader.require('ember-dev/test-helper/setup-qunit')['default'];
+ var testHelpers = new EmberDevTestHelperAssert(window.Ember, EmberDev.runningProdBuild);
+ setupQUnit(testHelpers, QUnit);
+ window.module = QUnit.module;

+ // Tests should time out after 5 seconds
+ QUnit.config.testTimeout = 5000;
+ // Hide passed tests by default
+ QUnit.config.hidepassed = true;

@seanpdoyle
Copy link
Contributor Author

@stefanpenner still getting the same node 0.12.x Windows failure.

Has the landscape changed at all in the last few months since I've worked on this?

@rwjblue
Copy link
Member

rwjblue commented Jun 4, 2015

@seanpdoyle - Not that I am aware of, but we did miss you. 😸

@seanpdoyle
Copy link
Contributor Author

@rwjblue I would love to get this merged during this pre-June 12th push.

@seanpdoyle
Copy link
Contributor Author

@rwjblue @stefanpenner new and exciting failure:

› node -v
v0.10.38
› ember test
Future versions of Ember CLI will not support v0.10.38. Please update to Node 0.12 or io.js.
version: 0.2.7
Built project successfully. Stored in "/Users/Sean/src/ember-data/tmp/class-tests_dist-PTuvffjr.tmp".
dyld: lazy symbol binding failed: Symbol not found: _node_module_register
  Referenced from: /Users/Sean/src/ember-data/node_modules/ember-cli/node_modules/testem/node_modules/socket.io/node_modules/engine.io/node_modules/ws/build/Release/bufferutil.node
  Expected in: dynamic lookup

dyld: Symbol not found: _node_module_register
  Referenced from: /Users/Sean/src/ember-data/node_modules/ember-cli/node_modules/testem/node_modules/socket.io/node_modules/engine.io/node_modules/ws/build/Release/bufferutil.node
  Expected in: dynamic lookup

zsh: trace trap  ember test

@wecc
Copy link
Contributor

wecc commented Dec 3, 2015

I'm gonna go ahead and close this since ED now has been addonified.

@seanpdoyle Really appreciate your work on this even though it didn't get merged 👍

@wecc wecc closed this Dec 3, 2015
@seanpdoyle
Copy link
Contributor Author

👏 I'm glad that's the direction this library has gone.

@seanpdoyle seanpdoyle deleted the use-emberjs-build branch December 3, 2015 15:21
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants