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

Add ES6 Import Validation #209

Merged
merged 1 commit into from
Apr 9, 2014
Merged

Conversation

jgable
Copy link
Contributor

@jgable jgable commented Mar 31, 2014

Progress on #36

  • Add dependency broccoli-es6-import-validate
  • Add validateJs tree (only when env !== 'production')

Wanted to get this up for review. I've tested by running locally with npm link, creating a new project with ember new ember-test then ember build and ember server in the new project.

I tried to test out importing from a non existent module like import Blah from 'ember-test/something/missing' but something is failing with an ENOENT because that file doesn't exist that is being imported from. But, when I tested by adding an extra named import like

import {
  test,
  moduleFor,
  blah // <-- doesn't exist
} from 'ember-qunit';

I get a proper warning from the validationJs tree.

@stefanpenner
Copy link
Contributor

i think the ENOENT error should be presenting in a more useful way by broccoli (cc @joliss)

The rest seems awesome.

whitelist: {
'ember/resolver': ['default'],
'ember-qunit': [
'globalize',
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be cool if some conventions for vendor'd addons could exist to learn these import/exports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a side effect of mixing your module loader systems. ember-qunit is not really an ES6 module when the page loads, but it's imported like one and your sort of magically depending on the fact that when it's all transpiled to amd that import is becoming a require('ember-qunit') call.

You could maybe do a relative import path to the actual vendor file in '../vendor/ember-qunit/lib/main' but then your import from 'ember' is gonna fail so you have to do it all the way down.

At Sprout we handle vendor files that are not ES6 modules a little differently, we basically rely on them being on the page before hand (usually concatenated into a single vendors.js file) and then write an ES6 shimming vendor/libraries file:

/* File: script/vendor/libraries.js */

// Just in case something wants to pass in a custom globals val
var globals = globals || window;

export var jQuery = globals.jQuery;
export var $ = globals.jQuery;
export var underscore = globals._;
export var lodash = globals._;
export var _ = globals._;
export var Backbone = globals.Backbone;
export var icanhaz = globals.ich;
export var ich = globals.ich;
export var cconsole = globals.cconsole;
export var Modernizr = globals.Modernizr;
export var WallTime = globals.WallTime;
export var TwitterCldr = globals.TwitterCldr;
export var twttr = globals.twttr;
export var Hogan = globals.Hogan;

Then from other files we import with:

import { _, Backbone } from 'vendor/libraries';

Of course, you could always write individual shims per library sort of like requirejs makes it easy to do. At any rate, this lets us have a single flexibility point and keep our module "philosophy" coherent and staying in ES6 land. I think it would be nice to have a native shimming capability in ES6, I might bring it up on esdiscuss.

@stefanpenner
Copy link
Contributor

this looks great, when I'm back in NYC tomorrow I plan to give a whirl. Do you think you can rebase + update accordingly?

- Add dependency broccoli-es6-import-validate
- Add validateJs tree (only when env !== 'production')
@jgable
Copy link
Contributor Author

jgable commented Apr 7, 2014

Updated

  • Rebased on master
  • Updated to version 0.1.0 of broccoli-es6-import-validate with latest Broccoli updates

@stefanpenner
Copy link
Contributor

rerunning the failed test

@stefanpenner
Copy link
Contributor

@jgable is this good to go?

@jgable
Copy link
Contributor Author

jgable commented Apr 7, 2014

I believe so. Tested out with same test cases as before and they seemed to work.

@stefanpenner
Copy link
Contributor

@jgable awesome, I'll merge it in after work :)

Thanks man!

@stefanpenner
Copy link
Contributor

just tried it out, its pretty good. :)

stefanpenner added a commit that referenced this pull request Apr 9, 2014
@stefanpenner stefanpenner merged commit 066524d into ember-cli:master Apr 9, 2014
@javierjulio
Copy link

I'm still seeing the ENOENT error rather than any helpful validation. For example, in my ember-cli-todomvc project I'm porting over Stefan's tests from his EAK implementation of TodoMVC but I'm getting an import error that my app route file isn't found. I could very well be doing something wrong but this issue is the closest I've seen to what I'm experiencing. I've updated ember-cli to 0.23 and run ember init to update the project files. My project has all the latest commits so you'll be able to see all the changes there.

I'm starting off with just a simple route test. The module prefix is ember-cli-todomvc. In the app folder I have /app/routes/todos.js and in the related test at /tests/unit/routes/todos-test.js I have an import statement with the following:

import TodosRoute from 'ember-cli-todomvc/routes/todos';
// ...

Running ember build I get the following error:

Error: ENOENT, no such file or directory 'tmp/tree_merger-tmp_dest_dir-7aROyJaO.tmp/ember-cli-todomvc/routes/todos.js'

I've tried a few variations of the import statement path but each one gives the same error. Any idea what I could be doing wrong?

@stefanpenner
Copy link
Contributor

@javierjulio that validation only works for properties missing on existing modules.

The error you see is broccoli telling you the entire module is missing from the tree.

Currently test + app are separate trees, I believe the goal will be make an app tree, and a app + test tree. Which should mitigate this issue

(cc @rjackson )

@CapyTheBeara
Copy link
Contributor

@javierjulio see #274 for more information on your issue. There is also a reference to a Brocfile that may serve as a temporary workaround for you.

homu added a commit that referenced this pull request Apr 19, 2016
Update fs-extra to version 0.28.0 🚀

Hello 👋

🚀🚀🚀

[fs-extra](https://www.npmjs.com/package/fs-extra) just published its new version 0.28.0, which **is not covered by your current version range**.

If this pull request passes your tests you can publish your software with the latest version of fs-extra – otherwise use this branch to work on adaptions and fixes.

Happy fixing and merging 🌴

---
The new version differs by 13 commits .

- [`ef35b70`](jprichardson/node-fs-extra@ef35b70) `0.28.0`
- [`8c56e5b`](jprichardson/node-fs-extra@8c56e5b) `(Closes #209, Closes #237) lib/mkdirs: if invalid path char, return callback OR  throw err`
- [`a0cb04b`](jprichardson/node-fs-extra@a0cb04b) `(Closes #93) libs/mkdirs: prevent stack overflow if drive is not mounted in Windows`
- [`76dcfa9`](jprichardson/node-fs-extra@76dcfa9) `lib/mkdirs/tests/root: skip if network drive`
- [`f5c64d5`](jprichardson/node-fs-extra@f5c64d5) `gitignore: add npm debug`
- [`5747fb3`](jprichardson/node-fs-extra@5747fb3) `(Closes #192) removed createOutputStream()`
- [`2a5e355`](jprichardson/node-fs-extra@2a5e355) `readme: update note about hacking on fs-extra`
- [`5652b96`](jprichardson/node-fs-extra@5652b96) `changelog: update issue links (auto generated)`
- [`e2a7dae`](jprichardson/node-fs-extra@e2a7dae) `appveyor: node v5`
- [`e8d2949`](jprichardson/node-fs-extra@e8d2949) `package: update travis`
- [`a524434`](jprichardson/node-fs-extra@a524434) `readme: fix date of note`
- [`e21cc16`](jprichardson/node-fs-extra@e21cc16) `readme: note about dropping old Node.js`
- [`ad98995`](jprichardson/node-fs-extra@ad98995) `readme: update`

See the [full diff](jprichardson/node-fs-extra@6fb9fc7...ef35b70).

---
This pull request was created by [greenkeeper.io](http://greenkeeper.io/).
It keeps your software up to date, all the time.

<sub>
Tired of seeing this sponsor message? Upgrade to the supporter plan!
You'll also get your pull requests faster ⚡
</sub>
homu added a commit that referenced this pull request Apr 19, 2016
Update fs-extra to version 0.28.0 🚀

Hello 👋

🚀🚀🚀

[fs-extra](https://www.npmjs.com/package/fs-extra) just published its new version 0.28.0, which **is not covered by your current version range**.

If this pull request passes your tests you can publish your software with the latest version of fs-extra – otherwise use this branch to work on adaptions and fixes.

Happy fixing and merging 🌴

---
The new version differs by 13 commits .

- [`ef35b70`](jprichardson/node-fs-extra@ef35b70) `0.28.0`
- [`8c56e5b`](jprichardson/node-fs-extra@8c56e5b) `(Closes #209, Closes #237) lib/mkdirs: if invalid path char, return callback OR  throw err`
- [`a0cb04b`](jprichardson/node-fs-extra@a0cb04b) `(Closes #93) libs/mkdirs: prevent stack overflow if drive is not mounted in Windows`
- [`76dcfa9`](jprichardson/node-fs-extra@76dcfa9) `lib/mkdirs/tests/root: skip if network drive`
- [`f5c64d5`](jprichardson/node-fs-extra@f5c64d5) `gitignore: add npm debug`
- [`5747fb3`](jprichardson/node-fs-extra@5747fb3) `(Closes #192) removed createOutputStream()`
- [`2a5e355`](jprichardson/node-fs-extra@2a5e355) `readme: update note about hacking on fs-extra`
- [`5652b96`](jprichardson/node-fs-extra@5652b96) `changelog: update issue links (auto generated)`
- [`e2a7dae`](jprichardson/node-fs-extra@e2a7dae) `appveyor: node v5`
- [`e8d2949`](jprichardson/node-fs-extra@e8d2949) `package: update travis`
- [`a524434`](jprichardson/node-fs-extra@a524434) `readme: fix date of note`
- [`e21cc16`](jprichardson/node-fs-extra@e21cc16) `readme: note about dropping old Node.js`
- [`ad98995`](jprichardson/node-fs-extra@ad98995) `readme: update`

See the [full diff](jprichardson/node-fs-extra@6fb9fc7...ef35b70).

---
This pull request was created by [greenkeeper.io](http://greenkeeper.io/).
It keeps your software up to date, all the time.

<sub>
Tired of seeing this sponsor message? Upgrade to the supporter plan!
You'll also get your pull requests faster ⚡
</sub>
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.

None yet

4 participants