-
Notifications
You must be signed in to change notification settings - Fork 53
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
WIP: Rework examples, fix bugs, review i18n #339
base: dev
Are you sure you want to change the base?
Conversation
* Remove /hpi/auth/token, make it optional via login * Move @nuxt/axios to dependencies, otherwise it breaks
for (const activity of values) { | ||
state.activities.push(Object.assign({}, activity)) | ||
state.activities.push({...activity}) |
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.
Moved to review comment.
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.
Moved to review comment.
<el-radio | ||
label="activity.priority.low" | ||
> | ||
{{$t('activity.priority.low')}} |
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.
Element UI uses label for both value and label.
At least it allows using default slot for i18n.
Also ensure that token Notification is shown not only at home.
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.
Here are my commentary to explain the changes I've made.
.vscode/launch.json
Outdated
@@ -14,7 +14,8 @@ | |||
"sourceMaps": true, | |||
"env": { | |||
"NODE_ENV": "development", | |||
"DEBUG": "nuxt:*" | |||
"DEBUG": "nuxt:*,koa:*", |
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 wished we could have more logging and be able to configure through this here.
So far, it seems only console.log
works, but then it's hard to manage in code by (un|)commenting.
.vscode/launch.json
Outdated
"NODE_ENV": "", | ||
"DEBUG": "nuxt:*,koa:*" | ||
} | ||
}, |
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 will be useful for debugging at build.
@@ -117,12 +156,12 @@ const ExampleGetter = namespace('examples/index', Getter) | |||
default () { | |||
return { | |||
account: '', | |||
region: 'activity.city.ly', | |||
city: 'activity.city.ly', |
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 wasn't consistent. Region or city, or area?
@@ -180,7 +220,7 @@ export default class NewActivity extends Vue { | |||
|
|||
this.$refs[formName].validate((valid) => { | |||
if (valid) { | |||
this.$store.dispatch('examples/activity/add', this.formData) | |||
this.addActivity(this.formData) |
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.
Encapsulate it back to the Store
title, | ||
onClick: function notifyOnClickHandler () { | ||
this.$router.push('/session') | ||
}.bind(this) |
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.
Maybe there's a better way
let path = query.page || '/' | ||
let authenticated = await store.getters['session/authenticated'] | ||
if (authenticated) { | ||
redirect(path) |
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.
So we can redirect to /
or elsewhere (e.g. bookmarked link as /login?page=%2Fabout
=> /about
if already signed in and accessed /login
.
// const API_HOST = process.env.API_HOST || '127.0.0.1' | ||
// const API_PREFIX = process.env.API_PREFIX || '/' | ||
|
||
const AXIOS_TIMEOUT = process.env.AXIOS_TIMEOUT || consts.AXIOS_DEFAULT_TIMEOUT |
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.
Maybe use different naming convention?
const AXIOS_DEFAULT_TIMEOUT = process.env.AXIOS_DEFAULT_TIMEOUT || consts.AXIOS_DEFAULT_TIMEOUT
for (const activity of values) { | ||
state.activities.push(Object.assign({}, activity)) | ||
state.activities.push({...activity}) |
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 will need refactoring.
I'm unsure the recommended way with Element-UI and table rows.
My gut feeling tells me it's better to have an object in the Vuex store, with an unique identifier as a key, so we can iterate.
Yet give an array when asked for the entries.
Because as this is, at any dispatch, the store will grow. With duplicates and all.
What's your feeling about that @clarkdo ?
This is refered to as The examples are not updated, but always appended with duplicates in BUGS in PR detail
server/api/index.js
Outdated
|
||
if (consts.SHOW_EXAMPLES === true) { | ||
if (process.env.SHOW_EXAMPLES === 'true') { |
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.
Would that be OK like this, do you see a problem using this pattern?
ctx.status = 200 | ||
ctx.body = body | ||
}) | ||
|
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.
We don't need this anymore.
The only time we should receive token is when we got a response from the authentication.
@renoirb Thank you so much for the fantastic changes, I'll look into it asap. BTW, could you have a look at the build failure of the pr? |
Hey @clarkdo, Sure, I'll take a look! But I'll make sure build passes too! |
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.
Here are some more comments from code I've backported from my project at my employer.
"DEBUG": "nuxt:*" | ||
"DEBUG": "nuxt:*,axios:*,koa:*,koa-session:*,koa-router:*,follow-redirects:*", | ||
"SHOW_EXAMPLES": "true", | ||
"LOG_DIR": "${workspaceRoot}/logs" |
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 an unexpected Gem, @clarkdo!
I knew it was somehow redundant to do console.log with bunyan but I couldn't see how to use. With this, we could have VSCode handle logs. And make other users of Hare aware of this feature.
// An array of any paths to partially persist the state. | ||
// If no paths are given, the complete state is persisted. | ||
paths: [ | ||
'menu.hidden', |
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.
Set in place persistedstate for UI preferences in localStorage.
We could have a persistedstate key for different use-case, e.g. a datatable filter preference, etc.
A in-app message translation workspace, storing locally edited translations prior to committing it to respective translation data store.
prefix: consts.BASE_API | ||
}) | ||
|
||
router.get('/env', async ctx => { |
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.
That way, we could have a multi-level deployment and have Nuxt read from Side-Car, instead.
Note that I couldn't make @nuxtjs/dotenv work properly from Nuxt side.
Sure thing, when managing many deployments of the same app, and dynamic monitoring system, deployment specific toggles could be discovered from here.
What's your opinion, @clarkdo?
'M2NhIiwiY2xpZW50X2lkIjoidGF0LWNsaWVudCJ9.' + | ||
'ovWxqcBptquNR5QUBz1it2Z3Fr0OxMvWsnXHIHTcliI' | ||
'accept-language': 'zh', | ||
Cookie: 'hare:sess=eyJjYXB0Y2hhIjoiR2hDTCIsImp3dCI6ImV5SmhiR2NpT2lKSVV6STFOaUlzSW5SNWNDSTZJa3BYVkNKOS5leUpoZFdRaU9sc2lZbUZ6SWwwc0luVnpaWEpmYm1GdFpTSTZJbUZrYldsdUlpd2ljMk52Y0dVaU9sc2ljbVZoWkNKZExDSmxlSEFpT2prNU9UazVPVGs1T1RrNU9Ua3NJblZ6WlhKSlpDSTZJalF3TWpnNFlqZGxOV0pqWkRjM016TXdNVFZpWTJRM1ptUTNNakl3TURBeElpd2lZWFYwYUc5eWFYUnBaWE1pT2xzaVlXUnRhVzRpWFN3aWFuUnBJam9pTnpKbFl6TmpORE10TURNd1lTMDBNV1ZrTFdGaVlqSXRZamRoTWpZNU5UQTJPVEl6SWl3aVkyeHBaVzUwWDJsa0lqb2lZbUZ6TFdOc2FXVnVkQ0o5LnV3eXd6aU5ldEh5ZlNkaXFjSnQ2WFVHeTRWX1dZSFI0SzZsN09QMlZCOUkiLCJfZXhwaXJlIjoxNTI4NTYyNDA0NDkwLCJfbWF4QWdlIjo4NjQwMDAwMH0=; hare:sess.sig=0gGUUVRpb3VRbsj8ibRHRXlie30' |
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.
Maybe the tes fails because of session HTTP header managed differently Koa side now.
Koa only looks at Cookie, and only the one it created.
$axios | ||
}) => { | ||
/* | ||
if (!process.client) { |
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 still has to be figured out.
'ENDPOINT_BACKEND_VALIDATE', | ||
// See also server/utils/environment-variables.js | ||
'LB_ADDR', | ||
'SHOW_EXAMPLES' |
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 still unsure how to use this in Nuxt side. (incomplete)
An alternate would be to use for Nuxt and environment variable injection, either this and/or with an environment variable exposition Side-Car endpoint (see /hpi/env
).
A sure thing would be that private data would NOT BE a good fit for unauthenticated /hpi/env
, and for Nuxt side, this module here would be a good fit.
"dev:pm2": "pm2 start yarn --name=hare -- dev", | ||
"test": "yarn lint && yarn build:client && ava --verbose --serial ", | ||
"test": "yarn lint && yarn build:client", | ||
"test:FIXME": "yarn lint && yarn build:client && ava --verbose --serial ", |
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.
Sorry about that.
# NOTE: Work needs to be done so hare logs could go there too. | ||
# And we should review use of cross-env to use env-cmd | ||
# https://github.com/kentcdodds/cross-env#other-solutions | ||
#DEBUG=nuxt:*,axios:*,koa:*,koa-session:*,koa-router:*,follow-redirects:* |
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 so cool!
Maybe its better to use Environment Variables instead of loading a long string in packages.json
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.
HOST is needed inside the Docker image, I should say.
HOST=0.0.0.0 | ||
|
||
# Might be renamed to ENDPOINT_BASEURL | ||
LB_ADDR=http://example.private.local:8080 |
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 I mean is that LB_ADDR might be better renamed to ???_BASEURL
.
In other projects, I see the convention of ADDR as an IPv4/IPv6 address, not a string that starts by http and has no trailing slash.
Also, maybe it should be mentioning about the fact that this endpoint SHOULD BE from a private network, only accessible from Nuxt (i.e. not publicly exposed).
Opinion @clarkdo ?
LB_ADDR=http://example.private.local:8080 | ||
|
||
ENDPOINT_BACKEND_AUTH=/your/own/namespace/oauth/token | ||
ENDPOINT_BACKEND_VALIDATE=/your/own/namespace/user/validate |
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.
My BACKEND endponts are likely not the same as yours :)
4d86b41
to
8e2e97c
Compare
FYI, @clarkdo, I'm having issues running the service after a build when I tell which baseURL to use, it still does not use it. To see what's missing, I've created an experiment repository, I have two orphaned branches and I thought you should know about them. Branches:
Maybe it is by design (or because we run Koa/Nuxt side by side), that its impossible to tell dynamically which Mind to take a look? For the record, it seems to be the same issue at play from the current
And we see any XHR failing when doing Notice the Network error and |
Related to last comment, I could at last make it work (see branch rework-hare-20180619). Maybe that should be handled differently though. Setup
Intended use is from within a container (Dockerfile not provided here) where we'd run the steps above to build the service image. The docker service would be started using a line similar to what's in start (see below)
You should be able to go back and forth to Index and About page, and click on an Element UI button with a "Click me!" caption and see an XHR request from Possibly useful links |
@renoirb I've made some changes for upgrading nuxt and refactoring, so the dir structure has been changed, sorry for the inconvenience, are you going to continue working on this pr ? If anything I can help, feel free to contact me. |
You can close it. Also, I was wondering if you could document how you plan on using with Koa, is it planned to be used as an APi (hpi), AND a Nuxt serverMiddleware ? Making changes to Koa logic takes time.
If you want. I can make a koa microservice. Only for the data. We could tell that service where to authenticate, where to fetch language definition, how to fill a user session data (based on outcome of authentication, exposed only if cookie is valid, ready for @nuxt/axios, and Vuex to setup vuex store for current user), etc. |
Related to loading i18n. I've started something (that I can now do better) in https://github.com/renoirb/experiments-nuxtjs-i18n-element-scss-201809 feel free to pick stuff from it. |
FYI, I've played a bit with Vuex and TypeScript. |
(It's a bit off-topic, but somewhat related to this PR that's not moving.) Hi @clarkdo, I wanted to give you a small note that I have been thinking about how to expand on the architectural patterns and review how hare is made. I've spend a few hours in the last days and I'd like to show you the progress. Idea is that we could make hare be two separate packages ("apps") that can be published, and other co-dependencies can be either other npm packages, or handled from a mono-repo Have a look at this private repo and README: https://github.com/renoirb/hare2/blob/master/apps/side-car/README.md and come back to me using your favourite communication medium :) |
Work in progress! (and so this PR message gets edited as work is still in progress)
PR Objectives
See Questions below, they might guide me in a way you'll be OK with this project's design goal and scope.
yarn build
withNODE_ENV="production"
)server/locales/*.json
, orclient/locales/*.json
. (See renoirb/experiments-nuxt-i18n-remote)Bugs
The examples are not updated, but always appended with duplicates
Currently, there is no duplicate verification nor prevent loading if already there.
Issue when reloading page with data read from side-car
When we're on a view that is built based on data coming from Koa, when we reload, we get a NuxtError telling
127.0.0.1:80
is not available.In other words, Axios/Nuxt would loose the proper port and get Axios' default setting that isn't port 3000 anymore
Reproduction:
Questions
How would you have runtime configuration environment variables set?
For instance; After production build, when running, and want to tell to read data from something available to that node's internal network.
Would you use nuxt-community/dotenv-module, or selectively declare which are applicable via something like rolodato/dotenv-safe ?
How do you see localization and build time?
Localization messages may be needed in both
client/
andserver/
code.What was your plan about this, or what do you prefer?
And how about loading only what's needed, at the many places we'd have to inject it (e.g. Element UI' at
Vue.use(Element, {...})
, Nuxt, and so on.)Did you plan splitting data mocking from what Nuxt reads from Koa
I see two use-cases in
server/
.1. "Mocking", where we statically serve data for demo purposes. (e.g.
/hpi/platform/uaano/oauth/validate
whenLB_ADDR
is the default)2. "Side-Car" (or BFF), where we get a simple
GET /hpi/auth/whois
and it abstracts out authentication and makes off-the-band HTTP calls to other data sources, and normalizes output for Nuxt to consume.Did you have plans, or lessons-learned you want to share here?
How do you handle Ava tests
Some rework needs to be done regarding session, maybe Vuex store calls too often at
/hpi/auth/whois
, tests times out. This has to be figured out.I've never used Ava so far (Only Mocha, Jest, etc). Looks good though.
Screenshots
1. Ability to get a copy of the JWT token after authentication
1.1. Check a box at login
1.2. After Sign-In Notification alert
1.3. Visualize in
/session
received data, including JWT token1.4. Get a note that the Vuex store does no longer have the JWT token, and requires authentication
2. Review and Improve examples views
2.1. Side-Car XML data output from
/hpi/examples/activity
2.2. Filled Vuex store, using data from Side-Car
Notice the link to

/hpi/examples/activity
2.3. Creating an Activity
2.4. Looking the new Activity Detail