Skip to content

Change app name to twiddle#394

Merged
Gaurav0 merged 1 commit intoember-cli:masterfrom
Gaurav0:addons3
Apr 12, 2016
Merged

Change app name to twiddle#394
Gaurav0 merged 1 commit intoember-cli:masterfrom
Gaurav0:addons3

Conversation

@Gaurav0
Copy link
Copy Markdown
Contributor

@Gaurav0 Gaurav0 commented Apr 12, 2016

No description provided.

@joostdevries
Copy link
Copy Markdown
Member

lgtm

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Apr 12, 2016

Hmm. Does this break the addon processing? I suspect that it needs to build the addons with the same app name...

If it doesn't break then have we ensured that the cache of prebuilt addons is cleared?

@joostdevries
Copy link
Copy Markdown
Member

@rwjblue that's the reason for the change ;-) The backend now uses twiddle as app name since app was not allowed as name by ember-cli.

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Apr 12, 2016

@joostdevries - And the cache of prebuilt addons was cleared?

@joostdevries
Copy link
Copy Markdown
Member

@rwjblue Yes. Because the build process was completely refactored to use docker instead of lambda (allowing git deps and muuuch faster build times and lower costs).

joostdevries/twiddle-backend#2

@alexspeller
Copy link
Copy Markdown
Member

Wait, am I missing something or will this break all existing twiddled that
have imports in them for local modules?
On Tue, 12 Apr 2016 at 14:37, Robert Jackson notifications@github.com
wrote:

@joostdevries https://github.com/joostdevries - And the cache of
prebuilt addons was cleared?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#394 (comment)

@joostdevries
Copy link
Copy Markdown
Member

@alexspeller the name was recently changed from demo-app to app so that breakage was already present at that point AFAIK.

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Apr 12, 2016

We should detect the twiddle.json version, and use the appropriate name.

@joostdevries
Copy link
Copy Markdown
Member

@rwjblue agreed but prefer to track that in isolated issue since breakage was already introduced.

@Gaurav0
Copy link
Copy Markdown
Contributor Author

Gaurav0 commented Apr 12, 2016

@alexspeller It doesn't break imports that use relative urls. We never made 'demo-app' name public.

@Gaurav0 Gaurav0 merged commit ed3d1f6 into ember-cli:master Apr 12, 2016
@Gaurav0 Gaurav0 deleted the addons3 branch April 12, 2016 13:44
@joostdevries
Copy link
Copy Markdown
Member

@rwjblue #41

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Apr 12, 2016

so that breakage was already present at that point AFAIK

The change from demo-app to app was over 10 days ago (released in https://github.com/ember-cli/ember-twiddle/releases/tag/v0.7.2 5 days ago). Anyone bit by this, likely now has to deal with this twice.

This tool is a large community resource, and should follow the same guidelines on breakages as Ember itself. The change to app should have been reverted as soon as it was clear that this broke existing twiddles and the solution I proposed (and you created #41 to track) should have been implemented before introducing yet another breaking change.

@joostdevries
Copy link
Copy Markdown
Member

@rwjblue agreed. Let's hold off on releasing untill we fix #41

@joostdevries
Copy link
Copy Markdown
Member

or revert to demo-app

@joostdevries
Copy link
Copy Markdown
Member

@Gaurav0 we could also just do a very simple replace(/demo-app/g, "twiddle") in the UI only I guess

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.

4 participants