Fix all the things by stefanpenner · Pull Request #100 · ef4/ember-browserify

stefanpenner · 2016-09-13T23:03:44Z

TL;DR this opens support (improved support, not quite perfect) for add-on and nested add-on usage.

The aim is to mangle imports/exports such that, each browserify bundle will uniquely identify each module by version. The associated importing module will also refer to that version. This will in-theory allow duplicate versions to co-exist. We would prefer this not to work, but shrug.... 🔥

Correctly resolve module versions.
Correctly Browserify each dependency.
Support multiple browserify/browserify.js imports.
Figure out what happens with the test tree.
Confirm works for nested imports.
Confirm works with engines.

POC
remove ember-cli 1.x support
CachingBrowserify
- needs to pass-through all files. (StubGenerator now also rewrites importers)
- use BroccoliPlugin (it needs to produce stable output) – this is optional, can happen later.
StubGenerator
- safely rewrite app code imports (no regexp soup)
- resolve npm dependency versions
EmberAddon

nathanhammond · 2016-09-16T21:01:01Z

tests/dummy/lib/outdated/app/utils/floor-outdated.js

-export default function() {
-  return flooring();
-}
+export { default } from 'outdated/utils/floor-type';


This is intentionally different than the addon module and should not re-export the module from the addon.

please coordinate before doing work on this branch. I am in the process of fixing these issues.

…cting them itself.

@nathanhammond

@nathanhammond & @stefanpenner

This is to allow StubGenerator to rewrite app files which import npm modules. Allowing them to uniquely address Browserify modules.

* browserify_stubs needs to be provided to CachingBrowserify * everything else, is merged with the existing tree. This should only include the files that contain `npm:x` import statements.

stefanpenner · 2016-09-16T21:48:26Z

@nathanhammond the tests have been updated to work with some known add-on limitations. There still exist further issues, but this now works around a resolving bug in ember-cli. So it is at-least closing to possibly working.

stefanpenner · 2016-09-16T21:58:11Z

Im heading to a meeting, and wont be looking at this for the next hour or so. My latest changes have been updated.

Please review the new lib structure, especially how in-repo modules dependencies are setup. Including that they declare their dependency on ember-browserify itself (this way ember browserify atleast runs for them).

There are more issues yet, although this is getting closer...

Exists in tests/dummy/app/routes/application.js This reverts commit d25ff36.

Rework tests, add reexports scenario.

nathanhammond · 2016-09-19T04:35:32Z

The next step on this, which I've started, is to switch to pushing a preprocessor into the parent addon and away from postprocessTree. See #109.

Move to preprocessor pattern.

stefanpenner · 2016-09-20T15:34:40Z

package.json

+  "ember-addon": {
+    "configPath": "tests/dummy/config",
+    "paths": [
+      "node_modules/modern",


why is this not tests/dummy/lib/modern and tests/dumy/lib/outdated ?

Because by the time we actually need them in test they're present in node_modules because of the link behavior we use. Probably results in duplication, I need to think about this.

I believe we should be explicit, the indirection via symlink I think is unneeded.

…coming from.

stefanpenner · 2016-09-20T15:54:35Z

Some notes: browserify/browserify.js are being written (i believe):

for each bundle we expect 👍
for scenarios where no bundle exists 👎
for each bundle we expect, it seems we only get the last one 👎

We most likely need to address:

provide a non-colliding name to the file we intend to import

Something to consider:

1 bundle per dependency, rather then 1 bundle per host
- this would prevent us from having duplicate top level npm dependencies, but would mean per host bundles may not be as ideal.

nathanhammond · 2016-09-20T16:08:07Z

lib/stub-generator.js

+        var needsImportsReWritten = false;

        Object.keys(imports).forEach(function(name) {
+          needsImportsReWritten = true;


Do we want to try comparing the imports object across runs?

ya, i'll fix in the work im about todo.

stefanpenner · 2016-09-20T18:19:26Z

lib/index.js

+    toTree: function(tree) {
+      var type = 'js';
+      var root = findRoot(instance);
+      var target = findTarget(instance);


I believe this is already via the add-on API, we should use that reference to it instead.

stefanpenner · 2016-09-20T18:20:59Z

lib/index.js

+    ext: 'js',
+    toTree: function(tree) {
+      var type = 'js';
+      var root = findRoot(instance);


I don't believe this is currently being used. We should remove it.

nathanhammond · 2016-09-22T16:53:22Z

Order of execution:

dummy treeForAddon
modern treeForAddon
modern preprocessor
outdated treeForAddon
outdated preprocessor
dummy treeForVendor
modern treeForVendor
outdated treeForVendor
dummy preprocessor (app)
dummy preprocessor (tests)

Can use treeForVendor to "correctly" provide Browserify modules for all things but the root. This will eliminate the need to use .import.

Looking into available workarounds for the root, whose preprocessors run far too late. Also investigating the degree of change to Ember CLI and risk in changing this behavior to match the nested story.

This was referenced Sep 13, 2016

Update "temporary" stop-gap for supporting nested addons #93

Closed

Support hoisting to nearest host. #99

Merged

stefanpenner force-pushed the fix-all-the-things branch 4 times, most recently from e6a84f7 to bee43a9 Compare September 14, 2016 21:08

nathanhammond reviewed Sep 16, 2016

View reviewed changes

stefanpenner and others added 23 commits September 16, 2016 14:45

[REFACTOR] stubs now gets imports provided via set, rather than extra…

78e9b9d

…cting them itself.

first pass of import/export identifier mangling

eae97f3

@nathanhammond & @stefanpenner

more ember-cli 1.x related removals

c388ac6

allow StubGenerator -> CachingBrowserify to overwrite output from app.

b4e6a04

This is to allow StubGenerator to rewrite app files which import npm modules. Allowing them to uniquely address Browserify modules.

remove more ember-cli 1.x stuff

b0387a9

StubGenerator output needs to be merged into tree

d29a10e

* browserify_stubs needs to be provided to CachingBrowserify * everything else, is merged with the existing tree. This should only include the files that contain `npm:x` import statements.

resolve actual versions

1532cac

refactor tests (split up concerns)

9a81644

Fixed test for multiple input rewrites.

71ab74f

add jshinting to tests

cdf0067

fix hinting

831f464

fixup spacing

ee94775

use chai-files

d30025b

tidy up test files

b3f7faa

more cleanup

75f48c1

Add in Ember Addon boilerplate.

6548bfa

Fix CI.

0683633

Add in VS Code debug profile for mocha.

f02b3a5

Use string positions to handle replacement.

8fa1b0d

Extract rewrite-imports.

2ef155c

Alphabetize package.json

74ab985

npm fixtures

b65c162

Set up top-level addon fixtures.

a7f4b5d

stefanpenner added 2 commits September 16, 2016 14:45

remove trailing whitespace

062f674

update example to work with in-repo addons

e3c8b59

stefanpenner force-pushed the fix-all-the-things branch from 1edc86e to e3c8b59 Compare September 16, 2016 21:46

update symlink/junction hack to work in more cases

2c686a0

nathanhammond added 4 commits September 16, 2016 15:41

Rework tests, add reexports scenario.

f3e0231

Properly set on prepublish.

59004b2

Revert "add a very simple/basic scenario (that works)"

69cc443

Exists in tests/dummy/app/routes/application.js This reverts commit d25ff36.

Structure for doing the right thing in the vendor hook.

772c859

nathanhammond mentioned this pull request Sep 18, 2016

Allow fullPaths to be configurable from the consuming app #108

Closed

Merge pull request #106 from nathanhammond/fix-all-the-things

80b0caf

Rework tests, add reexports scenario.

nathanhammond added 4 commits September 19, 2016 10:43

Partial rewrite of preprocessor approach.

0ef7aa4

Move away from postprocessTree.

b6f11dc

Properly set basedir per target.

80597f7

Merge pull request #109 from nathanhammond/fix-all-the-things

0f5a959

Move to preprocessor pattern.

stefanpenner commented Sep 20, 2016

View reviewed changes

stefanpenner added 2 commits September 20, 2016 08:41

add comments, making it easier to understand where stuff is actually …

03c9c95

…coming from.

only rewrite if rewriting is required

f5908b9

nathanhammond reviewed Sep 20, 2016

View reviewed changes

remove optimization for now. Bring back with tests.

a70c006

stefanpenner force-pushed the fix-all-the-things branch from e4b6e7f to a70c006 Compare September 20, 2016 18:05

stefanpenner commented Sep 20, 2016

View reviewed changes

Use the proper hook for setting up the registry.

e314119

nathanhammond mentioned this pull request Nov 17, 2016

[Ready for Review]: Fastboot build in ember-cli ember-cli/ember-cli#6395

Closed

6 tasks

stefanpenner mentioned this pull request Dec 3, 2016

Import syntax should "just work" for ES6 modules available via npm ember-cli/rfcs#84

Closed

nathanhammond mentioned this pull request May 22, 2017

Allow addon.include to change mutate built-in app.imports ember-cli/ember-cli#7060

Closed

3 tasks

Conversation

stefanpenner commented Sep 13, 2016 • edited by nathanhammond Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR this opens support (improved support, not quite perfect) for add-on and nested add-on usage.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanpenner commented Sep 16, 2016

Uh oh!

stefanpenner commented Sep 16, 2016

Uh oh!

nathanhammond commented Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanpenner commented Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond commented Sep 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stefanpenner commented Sep 13, 2016 •

edited by nathanhammond

Loading

nathanhammond commented Sep 19, 2016 •

edited

Loading

stefanpenner commented Sep 20, 2016 •

edited

Loading