search

ef4 / ember-browserify

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

Allow fullPaths to be configurable from the consuming app #108

Closed timiyay closed 7 years ago

timiyay commented 7 years ago

This PR solves the lack of configurability of fullPaths to support other production-like deployment environments, like staging.

See https://github.com/ef4/ember-browserify/issues/107

nathanhammond commented 7 years ago

This doesn't account for the fact that app isn't guaranteed to have a project property, engines, and nested addon use. The entire approach to browserify is about to change, see #100.

I'm closing this PR because I don't want to try to land any other changes before the reimagining is complete but I'm leaving open the issue. Sound reasonable?

timiyay commented 7 years ago

@nathanhammond yeah fair enough, I charged headlong into the naive fix (time from GH issue to PR = 8 mins), so I'm happy to defer completely to the maintainers.

My issue sprung up around a potentially-related problem, and configuring fullPaths may not be the fix anyway. For background, I'm trying to switch to ember-browserify, but my app dies if it has been built and deployed by Travis (rather than locally).

The crashes occur when mapbox-gl-js - which I require as an npm:mapbox-gl-js import - tries to create web workers. See https://github.com/mapbox/mapbox-gl-js/issues/3123#issuecomment-247180967 if curious.

stefanpenner commented 7 years ago

is there a reason why don't just always specify fullPaths to false?

ef4 commented 7 years ago

Yes, it's because browserify doesn't correctly do incremental rebuilds when fullPaths is false. It's a performance optimization for working in development.

This is why we only default fullPaths: true for non-production builds.

timiyay commented 7 years ago

Perhaps we could take the reverse approach, hardcoding fullPaths: true for development and test?

This would support arbitrary environments with fullPath: false, such production, staging, qa, etc

ef4 commented 7 years ago

@timiyay that is a misunderstanding of how ember-cli handles environment. The word is unfortunately confusing.

qa, staging, and production environments are all production builds as far as ember-cli is concerned. But they can be different deploy targets (in ember-cli-deploy parlance).

Attempting to extend ember-cli's list of environments beyond development/test/production is not supported and likely to break.

timiyay commented 7 years ago

Ah righto, I'll need to review our environment / deploy practices for supporting a staging environment.

Our setup is based on a late-2015 recommendation from ember-cli-deploy, on creating a new Ember CLI environment for staging. It appears this recommendation no longer appears in the ember-cli-deploy docs.