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

feat: support scoped bundle names #63

Merged
merged 1 commit into from Mar 19, 2018

Conversation

lennyburdette
Copy link
Contributor

When using an engine in a package called "@lennyburdette/my-engine", I would like to keep the following strings consistent:

  • The package name (for ember install @lennyburdette/my-engine).
  • The addon/engine name in index.js (for this.mount('@lennyburdette/my-engine', { path: 'my-engine' }).
  • The module prefix in config/environment (for import Foo from '@lennyburdette/my-engine').

Currently, ember-asset-loader generates the asset manifest using only the scope, not the full package name. This change detects package scopes and only generates bundles with full package names.

@lennyburdette
Copy link
Contributor Author

@rwjblue @trentmwillis Looks like I could use some help getting tests to pass here.

It appears that master started failing when jquery 3.3 was released:
https://travis-ci.org/ember-engines/ember-asset-loader/builds
https://blog.jquery.com/2018/01/19/jquery-3-3-0-a-fragrant-bouquet-of-deprecations-and-is-that-a-new-feature/

There's a note on the jquery release post that they are no longer testing in phantomjs 1.9. Should this project also drop support for that version?

The other failure is a node test exceeding the two second test timeout. This may be due to my changes, but I don't think so. The failing test is the first to use verifyInsertedManifest, which spins up a broccoli builder. I'm assuming the next two tests are able to take advantage of a broccoli cache so they're a bit faster. Is it reasonable to assume that a broccoli builder could take longer that 2000ms on an underpowered travis vm?

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Thanks for fixing CI in #66! Now that things are in order, we can start moving forward here...

* @param {string} entry
* @returns {BundleInfo}
*/
function bundleInfoFromAssetEntry(entry) {
Copy link
Member

Choose a reason for hiding this comment

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

I really like splitting this out into a stand alone function, which should make testing various combinations much easier. Can you export this function privately, and add some unit tests around expected inputs and outputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Adding tests gave me an opportunity to clean up some quirks that carried over from the original implementation.

e.g. "@lennyburdette/some-engine".
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Awesome thank you!

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

2 participants