-
Notifications
You must be signed in to change notification settings - Fork 27
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
[Breaking] refactor: drop mc-http-server, make --use-local-assets the default behavior #1787
Conversation
🦋 Changeset detectedLatest commit: 8faf3b3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/c6ghb31ei |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Wondering if we should provide potential security fixes and e.g. dependency updates for the mc-http-server for a bit longer?
Hmm you mean in the Docker image? |
Yes, or is it pretty much shallow? |
Btw, the current HTTP server version is actually broken (if you don't use the Given that we haven't had any bug report for the past months, I would assume that nobody is using that anyway. |
I don't think the conclusion of something broken should be to remove it entirely. I am open to remove the |
Yes and no. We can fix it (which is what I tried to do in the last hour) but doing so requires some (breaking) changes regardless (because of the way the application config is loaded). So I'd rather encourage people to use something else, or use another approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay then. The old versions used will continue to work if running. I guess we will educate people in the release notes how to migrate. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to removing the mc-http-server
, just wasn't expecting to initiate this discussion over a PR.
If it is a breaking change regardless, then sure we can remove it.
We'll have to make sure to provide a migration path.
My initial intention was not to remove it obviously. But by looking at what needs to be done it's just not really worth the effort. Hence the suggestion that we simply remove the package. We'll provide proper documentation to migrate of course. |
6bbad53
to
bd5e232
Compare
@@ -6,7 +6,8 @@ | |||
"scripts": { | |||
"build": "mc-scripts build", | |||
"start": "dotenv -- mc-scripts start", | |||
"start:prod:local": "NODE_ENV=production MC_APP_ENV=development dotenv -- mc-http-server --use-local-assets", | |||
"compile-html": "NODE_ENV=production dotenv -- mc-scripts compile-html", | |||
"start:prod:local": "NODE_ENV=production MC_APP_ENV=development dotenv -- mc-scripts compile-html && NODE_ENV=production npx serve -l 3001 public", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To run it locally in production mode, we can do it like this: compile it first, then serve the static file using serve
(or any other static server CLI).
@@ -6,7 +6,8 @@ | |||
"scripts": { | |||
"build": "mc-scripts build", | |||
"start": "dotenv -- mc-scripts start", | |||
"start:prod:local": "NODE_ENV=production MC_APP_ENV=development dotenv -- mc-http-server --use-local-assets", | |||
"compile-html": "NODE_ENV=production dotenv -- mc-scripts compile-html", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just added for convenience.
@@ -6,7 +6,9 @@ | |||
"scripts": { | |||
"build": "mc-scripts build", | |||
"start": "dotenv -- mc-scripts start", | |||
"start:prod:local": "NODE_ENV=production MC_APP_ENV=development dotenv -- mc-http-server --use-local-assets", | |||
"compile-html": "NODE_ENV=production dotenv -- mc-scripts compile-html", | |||
"compile-html:local": "NODE_ENV=production MC_APP_ENV=development dotenv -- mc-scripts compile-html --transformer @commercetools-frontend/mc-dev-authentication/transformer-local.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is turning out to be pretty nice. The mc-dev-authentication
package now implements a transformer-local.js
that compiles and copies the login/logout routes to the public
folder of the application.
This way, everything is still static and we can start the app in production mode locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is also nice is that we can reference npm packages/modules.
--transformer @commercetools-frontend/mc-dev-authentication/transformer-local.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a bit related to #1733.
We can have a package with pre-configured transformers, which people can also simply copy or extend.
…e with login and logout dev pages
e7c72af
to
09d74f7
Compare
@@ -1,19 +1,16 @@ | |||
--- | |||
date: 2020-10-12 | |||
title: Migrating to the new Apollo Client 3.0 | |||
title: Migrating to v17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed the release note, as it's about migrating to v17 now (which of course includes the apollo v3 migration).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice. The transform local is nice. Feels like it removes concepts and keeps things more concise.
Maybe we should add a note to the application config page that it doesn’t work with the http-server which was deprecated in v17. Just a thought.
…custom application locally
mc-scripts compile-html \ | ||
--transformer @commercetools-frontend/mc-dev-authentication/transformer-local.js | ||
|
||
mc-scripts serve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot that we need to serve the files as a SPA.
The serve
package has a --single
option that does that but then we loose the routes for the login/logout pages.
Therefore I decided to add a new serve
command to mc-scripts
, so that we can customize the rewrite routes.
A serve
command is something that is also used in other tools, like Gatsby.
In our case the mc-scripts serve
command starts an HTTP server to serve the files in the public
folder. The main use case for this is to start the Custom Application locally to test the production bundles.
const server = http.createServer((request, response) => { | ||
// You pass two more arguments for config and middleware | ||
// More details here: https://github.com/vercel/serve-handler#options | ||
return serveHandler(request, response, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
… default behavior (#1787) * refactor: drop mc-http-server, make --use-local-assets the default behavior * fix: ports * docs: remove references to http-server * docs: keep old pages path but link to other documents * docs: changeset * fix: use transformer-local for starting app locally in production mode with login and logout dev pages * fix(website): links * refactor: add mc-scripts serve command to serve the production-built custom application locally * chore: remove refs to mc-http-server
… default behavior (#1787) * refactor: drop mc-http-server, make --use-local-assets the default behavior * fix: ports * docs: remove references to http-server * docs: keep old pages path but link to other documents * docs: changeset * fix: use transformer-local for starting app locally in production mode with login and logout dev pages * fix(website): links * refactor: add mc-scripts serve command to serve the production-built custom application locally * chore: remove refs to mc-http-server
…f @formatjs/cli command (#1790) * [Breaking] Migrate packages to new @apollo/client (#1677) * refactor: migrate to new @apollo/client package * docs: breaking changes * fix: remove es6 import of apollo version * fix(apollo/cache): define type policies, remove unused key fields from product queries * feat: expose apollo hooks with context type bound to MC * docs: draft release notes for migrating to new apollo client * docs(website): divider for release notes content * refactor: remove unused type * fix: tests and build * fix: unused import * fix: test * test: fix leftovers * fix(website): text style workaround * chore: update @graphql-codegen/add to v2 * chore: rebase leftovers * docs(apollo-3): improve description on request context * docs: improve wording * refactor: use apollo-link-logger package * refactor(app-shell): allow to configure the apollo client (and cache) * test: fix assertions * fix: links * fix: prettier Co-authored-by: Adnan Asani <adnasa@users.noreply.github.com> * [Breaking] refactor: drop mc-http-server, make --use-local-assets the default behavior (#1787) * refactor: drop mc-http-server, make --use-local-assets the default behavior * fix: ports * docs: remove references to http-server * docs: keep old pages path but link to other documents * docs: changeset * fix: use transformer-local for starting app locally in production mode with login and logout dev pages * fix(website): links * refactor: add mc-scripts serve command to serve the production-built custom application locally * chore: remove refs to mc-http-server * refactor: remove mc-scripts extract-intl, document usage of @formatjs/cli command Co-authored-by: Adnan Asani <adnasa@users.noreply.github.com>
… default behavior (#1787) * refactor: drop mc-http-server, make --use-local-assets the default behavior * fix: ports * docs: remove references to http-server * docs: keep old pages path but link to other documents * docs: changeset * fix: use transformer-local for starting app locally in production mode with login and logout dev pages * fix(website): links * refactor: add mc-scripts serve command to serve the production-built custom application locally * chore: remove refs to mc-http-server
…f @formatjs/cli command (#1790) * [Breaking] Migrate packages to new @apollo/client (#1677) * refactor: migrate to new @apollo/client package * docs: breaking changes * fix: remove es6 import of apollo version * fix(apollo/cache): define type policies, remove unused key fields from product queries * feat: expose apollo hooks with context type bound to MC * docs: draft release notes for migrating to new apollo client * docs(website): divider for release notes content * refactor: remove unused type * fix: tests and build * fix: unused import * fix: test * test: fix leftovers * fix(website): text style workaround * chore: update @graphql-codegen/add to v2 * chore: rebase leftovers * docs(apollo-3): improve description on request context * docs: improve wording * refactor: use apollo-link-logger package * refactor(app-shell): allow to configure the apollo client (and cache) * test: fix assertions * fix: links * fix: prettier Co-authored-by: Adnan Asani <adnasa@users.noreply.github.com> * [Breaking] refactor: drop mc-http-server, make --use-local-assets the default behavior (#1787) * refactor: drop mc-http-server, make --use-local-assets the default behavior * fix: ports * docs: remove references to http-server * docs: keep old pages path but link to other documents * docs: changeset * fix: use transformer-local for starting app locally in production mode with login and logout dev pages * fix(website): links * refactor: add mc-scripts serve command to serve the production-built custom application locally * chore: remove refs to mc-http-server * refactor: remove mc-scripts extract-intl, document usage of @formatjs/cli command Co-authored-by: Adnan Asani <adnasa@users.noreply.github.com>
… default behavior (#1787) * refactor: drop mc-http-server, make --use-local-assets the default behavior * fix: ports * docs: remove references to http-server * docs: keep old pages path but link to other documents * docs: changeset * fix: use transformer-local for starting app locally in production mode with login and logout dev pages * fix(website): links * refactor: add mc-scripts serve command to serve the production-built custom application locally * chore: remove refs to mc-http-server
…f @formatjs/cli command (#1790) * [Breaking] Migrate packages to new @apollo/client (#1677) * refactor: migrate to new @apollo/client package * docs: breaking changes * fix: remove es6 import of apollo version * fix(apollo/cache): define type policies, remove unused key fields from product queries * feat: expose apollo hooks with context type bound to MC * docs: draft release notes for migrating to new apollo client * docs(website): divider for release notes content * refactor: remove unused type * fix: tests and build * fix: unused import * fix: test * test: fix leftovers * fix(website): text style workaround * chore: update @graphql-codegen/add to v2 * chore: rebase leftovers * docs(apollo-3): improve description on request context * docs: improve wording * refactor: use apollo-link-logger package * refactor(app-shell): allow to configure the apollo client (and cache) * test: fix assertions * fix: links * fix: prettier Co-authored-by: Adnan Asani <adnasa@users.noreply.github.com> * [Breaking] refactor: drop mc-http-server, make --use-local-assets the default behavior (#1787) * refactor: drop mc-http-server, make --use-local-assets the default behavior * fix: ports * docs: remove references to http-server * docs: keep old pages path but link to other documents * docs: changeset * fix: use transformer-local for starting app locally in production mode with login and logout dev pages * fix(website): links * refactor: add mc-scripts serve command to serve the production-built custom application locally * chore: remove refs to mc-http-server * refactor: remove mc-scripts extract-intl, document usage of @formatjs/cli command Co-authored-by: Adnan Asani <adnasa@users.noreply.github.com>
… default behavior (#1787) * refactor: drop mc-http-server, make --use-local-assets the default behavior * fix: ports * docs: remove references to http-server * docs: keep old pages path but link to other documents * docs: changeset * fix: use transformer-local for starting app locally in production mode with login and logout dev pages * fix(website): links * refactor: add mc-scripts serve command to serve the production-built custom application locally * chore: remove refs to mc-http-server
…f @formatjs/cli command (#1790) * [Breaking] Migrate packages to new @apollo/client (#1677) * refactor: migrate to new @apollo/client package * docs: breaking changes * fix: remove es6 import of apollo version * fix(apollo/cache): define type policies, remove unused key fields from product queries * feat: expose apollo hooks with context type bound to MC * docs: draft release notes for migrating to new apollo client * docs(website): divider for release notes content * refactor: remove unused type * fix: tests and build * fix: unused import * fix: test * test: fix leftovers * fix(website): text style workaround * chore: update @graphql-codegen/add to v2 * chore: rebase leftovers * docs(apollo-3): improve description on request context * docs: improve wording * refactor: use apollo-link-logger package * refactor(app-shell): allow to configure the apollo client (and cache) * test: fix assertions * fix: links * fix: prettier Co-authored-by: Adnan Asani <adnasa@users.noreply.github.com> * [Breaking] refactor: drop mc-http-server, make --use-local-assets the default behavior (#1787) * refactor: drop mc-http-server, make --use-local-assets the default behavior * fix: ports * docs: remove references to http-server * docs: keep old pages path but link to other documents * docs: changeset * fix: use transformer-local for starting app locally in production mode with login and logout dev pages * fix(website): links * refactor: add mc-scripts serve command to serve the production-built custom application locally * chore: remove refs to mc-http-server * refactor: remove mc-scripts extract-intl, document usage of @formatjs/cli command Co-authored-by: Adnan Asani <adnasa@users.noreply.github.com>
Ref #1769
TL;DR: The usage of the
--use-local-assets
flag is confusing and it turns out that this is what you need 99% of the time so we should make it the default behavior.I also suggest to remove the
mc-http-server
. I don't see a reason to keep maintaining and having it. People can runcompile-html
first, then serve the static files however they would like to.