ef4 / ember-browserify

ember-cli addon for easily loading CommonJS packages from npm via browserify.
MIT License
172 stars 28 forks source link

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

Closed seanjohnson08 closed 7 years ago

seanjohnson08 commented 7 years ago

When upgrading to 2.7.0, I'm seeing the "sourcemaps not defined" issue again. It seems that the previous stopgap wasn't as future-proof as we'd like it to be. :)

I've switched from duck-typing against app.import to app.options, since that's the accessor that fails every time this bug resurfaces.

Here's the previous PR: https://github.com/ef4/ember-browserify/pull/30/files

asakusuma commented 7 years ago

@seanjohnson08 can you confirm that this works pre 2.7.0?

stefanpenner commented 7 years ago

how does this deal with dupes of deps across nested addons?

caseklim commented 7 years ago

In regards to @asakusuma 's question, this was not entirely working before. We started experiencing this issue with ember-cli at 2.6.3, and bumping the version down to 2.6.2 temporarily solved the issue at the app level.

I'm now experiencing the issue again in an addon that uses ember-browserify where I pinned ember-cli to 2.6.2. When I hook up my app to an engine that uses the addon using ember-browserify I'm experiencing the error again.

caseklim commented 7 years ago

@stefanpenner can confirm that @seanjohnson08 's fix solved the error in my situation

stefanpenner commented 7 years ago

@stefanpenner can confirm that @seanjohnson08 's fix solved the error in my situation

This wasn't actually my question, this appears to be adding a new feature. One that opens the door to a currently unaddressed problem, which is how does one deal with duplicate dependencies from discrete add-ons. I don't believe we can accept this feature without an answer to ^.

@cklimkowsky if there was a regression in ember-cli 2.6.3, we should investigate it, and come up with a solution that doesn't require this new feature. Do you have a reproduction of this described failure?

caseklim commented 7 years ago

@stefanpenner I was just providing additional context. When you say a reproduction, do you mean with ember-cli 2.6.3 or the issue I described with my app/engine/addon situation?

stefanpenner commented 7 years ago

@cklimkowsky awesome, the context is much appreciated!

do you mean with ember-cli 2.6.3 or the issue I described with my app/engine/addon situation?

Yes, a reproduction for the 2.6.3 regression would be great, do you mind opening a new issue (on this repo) describing the symptoms? That way we can track the two issues independently.

stefanpenner commented 7 years ago

@cklimkowsky im also unsure if 2.6.2 and 2.6.3 resulted in any breaking changes: diff -> https://github.com/ember-cli/ember-cli/compare/v2.6.2...v2.6.3 Maybe its a different dependency?

caseklim commented 7 years ago

@stefanpenner hm, I just tried reproducing the 2.6.3 issue and I unfortunately can't 😕

asakusuma commented 7 years ago

@seanjohnson08 are you trying to upgrade to ember-cli 2.7 or ember 2.7?

cc @tjni

derrickshowers commented 7 years ago

@asakusuma @stefanpenner I've also run into this issue when bumping ember-cli to 2.7. No issues on 2.6.3.

stefanpenner commented 7 years ago

@derrickshowers can you share a reproduction, would love to dig in.

derrickshowers commented 7 years ago

@seanjohnson08 Have you had any luck (or know of a way) reproducing this on a fresh ember-cli app? Tried including ember-browserify as a dependency and a downstream dependency, but can't get to reproduce the error.

In the app I'm working with, it's reproducible every time I upgrade ember-cli to 2.7.0. 🤔

jwlawrence commented 7 years ago

Guess I'll jump on the wagon. I'm getting this error after updating ember-cli to 2.7.0 and running ember init with ember-browserify@1.1.12. Not sure what other information I can provide that would be helpful, but I didn't have this issue on ember-cli@2.6.2.

nathanhammond commented 7 years ago

100% of this thread is LinkedIn people. We know that this is completely broken. We'll have a story for this at some point next week.

stefanpenner commented 7 years ago

will be superseded by https://github.com/ef4/ember-browserify/pull/100