-
Notifications
You must be signed in to change notification settings - Fork 985
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
Fixes #21312 - components for formatting dates #5184
Conversation
Issues: #21312 |
index_prefix = File.basename(entry_path).gsub(/[_]?index[.]js$/, '') | ||
bundle = plugin_name.to_s.gsub(/core$/, '').gsub(/-|_|#{remove_bundle_name_plugins}/,'') | ||
|
||
if index_prefix == '' |
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.
nitpick: .blank?
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're using this helper also in plugin_webpack_directories.rb
where we don't have active support included.
config/webpack.config.js
Outdated
if (production) { | ||
publicPath = process.env.ASSET_PATH || '/webpack/'; | ||
} else { | ||
publicPath = process.env.ASSET_PATH || `http://${process.env.HOSTNAME}:${devServerPort}/webpack/`; |
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.
do you really need the entire url? I would prefer to avoid hardcoding the path if we URL scheme
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.
Unfortunately we really need the entire url. We have to pass the dev server port. Without that webpack fetches dynamic imports from wrong url.
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.
Seems my process.env.HOSTNAME
is undefined.
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.
How should it work in production? Do we need the ASSET_PATH
in production?
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.
Should consider adding this variable to Procfile and script/foreman-start-dev?
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'll try to tune the Procfile. Production will be served from /webpack/
as before. That branch is unchanged.
config/webpack.config.js
Outdated
|
||
filename: production ? '[name]-[chunkhash].js' : '[name].js' | ||
filename: production ? '[name]-[chunkhash].js' : '[name].js', | ||
chunkFilename: production ? '[name]-[chunkhash].js' : '[name].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.
what does this do?
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.
https://webpack.js.org/configuration/output/#output-chunkfilename
Name format for non-entry chunks created by webpack.
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 am getting all of those files into my public/webpack when building production:
0-03661867bb957b74fba9.js
1-1d5ccaf80730fcbd15a5.js
2-531ce93840f13a2bb530.js
3-0fe5d3ece4ad57ea7eb8.js
n-.....................js
28-.....................js
shouldn't it be en-[hash].js
/ pt_BR-[hash].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.
Does it mean plugins are also going to build those files?
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.
The dynamic chunks didn't have webpackChunkName
configured. I'll fix it.
@@ -28,7 +28,7 @@ export function templateSelected(item) { | |||
$('#storage_volumes .children_fields >.fields').remove(); | |||
$.each(result.volumes, function () { | |||
// Change variable name because 'interface' is a reserved keyword. | |||
this.disk_interface = this['interface']; | |||
this.disk_interface = this.interface; |
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.
not related?
@@ -6,4 +6,7 @@ export default { | |||
}); | |||
}, | |||
noop: Function.prototype, // empty function | |||
getDisplayName(Component) { | |||
return Component.displayName || Component.name || 'Component'; |
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.
isnt this the default (just with an Unknown string instead of Component?
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 not sure what are the defaults. I saw displayName
being undefined at some places. This approach is a React.js convention for HOCs: https://reactjs.org/docs/higher-order-components.html#convention-wrap-the-display-name-for-easy-debugging
@sharvit @amirfefer @waldenraines @danseethaler can you guys review please? |
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.
Looks good overall - just one question with the babel setup.
.babelrc
Outdated
"presets": ["env", "react"], | ||
"presets": [ | ||
"env", | ||
["es2015", {"modules": false}], |
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 pretty sure you can pass the {"modules": false}
into the env
preset. Is there a reason we're targeting 2015
only here?
package.json
Outdated
"babel-plugin-transform-class-properties": "^6.24.1", | ||
"babel-plugin-transform-object-assign": "^6.8.0", | ||
"babel-plugin-transform-object-rest-spread": "^6.8.0", | ||
"babel-preset-env": "^1.6.1", | ||
"babel-preset-es2015": "^6.24.1", |
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.
Same question as above about needing 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.
Nice work @tstrachota 👍
Good job implementing the async importers!
It's a significant and blessed change, so I hope it will merge soon even though I have many comments.
config/webpack.config.js
Outdated
if (production) { | ||
publicPath = process.env.ASSET_PATH || '/webpack/'; | ||
} else { | ||
publicPath = process.env.ASSET_PATH || `http://${process.env.HOSTNAME}:${devServerPort}/webpack/`; |
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.
Seems my process.env.HOSTNAME
is undefined.
config/webpack.config.js
Outdated
// Detect supported locales from folders in ./locale/ | ||
const localeDir = path.join(__dirname, '..', 'locale') | ||
const localeSubdirs = [ ...new Set(fs.readdirSync(localeDir).filter(f => fs.statSync(path.join(localeDir, f)).isDirectory())) ]; | ||
const supportedLocales = localeSubdirs.map(d => d.split('_')[0]).join('|') |
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.
Can you refactor those lines so it will be easy to understand?
config/webpack.config.js
Outdated
if (production) { | ||
publicPath = process.env.ASSET_PATH || '/webpack/'; | ||
} else { | ||
publicPath = process.env.ASSET_PATH || `http://${process.env.HOSTNAME}:${devServerPort}/webpack/`; |
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.
How should it work in production? Do we need the ASSET_PATH
in production?
config/webpack.config.js
Outdated
|
||
filename: production ? '[name]-[chunkhash].js' : '[name].js' | ||
filename: production ? '[name]-[chunkhash].js' : '[name].js', | ||
chunkFilename: production ? '[name]-[chunkhash].js' : '[name].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.
I am getting all of those files into my public/webpack when building production:
0-03661867bb957b74fba9.js
1-1d5ccaf80730fcbd15a5.js
2-531ce93840f13a2bb530.js
3-0fe5d3ece4ad57ea7eb8.js
n-.....................js
28-.....................js
shouldn't it be en-[hash].js
/ pt_BR-[hash].js
?
config/webpack.config.js
Outdated
if (production) { | ||
publicPath = process.env.ASSET_PATH || '/webpack/'; | ||
} else { | ||
publicPath = process.env.ASSET_PATH || `http://${process.env.HOSTNAME}:${devServerPort}/webpack/`; |
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.
Should consider adding this variable to Procfile and script/foreman-start-dev?
import { locale, ready as i18nReady, loaded as i18nLoaded } from './i18n'; | ||
import helpers from './helpers'; | ||
|
||
export function i18nProviderWrapper(initialNow = undefined) { |
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.
can use es6 syntax for the function?
Shouldn't the default value of initialNow
should be undefined
anyway?
super(props); | ||
if (!i18nLoaded) { | ||
i18nReady.then(() => { | ||
this.forceUpdate(); |
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 it make more sense to use an inner store instead forceUpdate
?
import { FormattedDate, intlShape } from 'react-intl'; | ||
import { timezone } from '../../../common/i18n'; | ||
|
||
class Date extends React.Component { |
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.
Doesn't it interfere with the native Date?
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.
+1
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 do you think of the name IsoDate
instead?
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 like IsoDate
, I'll change the names.
|
||
class Date extends React.Component { | ||
render() { | ||
if (this.props.data.date) { |
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.
Can do:
const { date, default } = this.props.data;
if (date) {
return ...;
}
return <span>{default}</span>;
import RelativeDateTime from './common/dates/RelativeDateTime'; | ||
import LongDateTime from './common/dates/LongDateTime'; | ||
import ShortDateTime from './common/dates/ShortDateTime'; | ||
import Date from './common/dates/Date'; |
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.
Afraid Date
interfere with the native Date
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's true. One has to import it under a different name if you plan to use them together. I couldn't find any better name. Suggestions?
Can you add the |
$.cookie('timezone', tz.name(), { path: '/', secure: location.protocol === 'https:' }); | ||
tfm.i18n.ready.then(() => { | ||
var tz = jstz.determine(); | ||
$.cookie('timezone', tz.name(), { path: '/', secure: location.protocol === 'https:' }); |
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.
hey @tstrachota ... maybe you can have some Cookies
with this PR? 🍪
https://github.com/js-cookie/js-cookie
wdyt?
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 in the future but not now... i dunno...
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.
Yes @priley86 we should actually do that, thanks for the advice 👍
I was also looking into that repository:
https://github.com/reactivestack/cookies
I think the js-cookie
is better for us, for now, i still don't see us using CookieProvider
or withCookie
wrapper.
We actually trying to get ride of jquery which is a long process and migrating the cookies is a good step.
I open an issue and i hope to do it soon:
http://projects.theforeman.org/issues/22506
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 am fine w/ react-cookie too... and yes, very much prefer that API down the road if it becomes an option. Universal cookies are even more tasty! They could stream via react-dom/server
within apollo-server
in an isomorphic way, you know? Some kind of strange renderToNodeStream
voodoo...haha
overall, i like the approach here w/ react-intl
and utilizing the componentRegistry. I noticed Twitter doing something very similar yesterday. I'm happy to move off of jquery as you have time...
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 ok with migrating from jquery cookies to some other solution in future, but it's out of the scope of this PR.
return <span />; | ||
} | ||
return ( | ||
<IntlProvider locale={i18n.locale} initialNow={initialNow}> |
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 think you can render messages on the IntlProvider
as well...
https://egghead.io/lessons/react-convert-a-hard-coded-string-using-react-intl-formattedmessage
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.
Yeah, I know about it. But we don't use react-intl for messages. It's one of the options for future, but we haven't decided what will be our approach yet. We can easily add it later.
import { FormattedDate, intlShape } from 'react-intl'; | ||
import { timezone } from '../../../common/i18n'; | ||
|
||
class LongDateTime extends React.Component { |
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 think all of these can be converted to stateless functional components.... at least my linter tells me that ;)
example: priley86/manageiq-v2v@9db79b6
ShortDateTime.propTypes = { | ||
data: PropTypes.shape({ | ||
date: PropTypes.any, | ||
defaultValue: PropTypes.string, |
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.
you are missing seconds
in propTypes here
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.
Updated
[test] |
Where are we with this? This is blocking Katello/katello#7001 which is in turn blocking some katello work which relies on the infrastructure in that PR. |
@tstrachota do you mind rebasing this please? |
@waldenraines rebased |
@ohadlevy what else is needed here? |
[test foreman] |
@waldenraines I didnt test it yet (lack of time) and the fact that @sharvit still didn't approve it? |
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.
@tstrachota I couldn't run it locally, can you check it please?
.babelrc
Outdated
"presets": ["env", "react"], | ||
"presets": [ | ||
"env", | ||
"es2015", |
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.
Does it really necessary? Shouldn't the env
provide the es-latest-stable
?
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.
When running tests, I am getting a bunch of those errors:
ERROR: Error: Couldn't find preset "es2015" relative to directory
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.
Same when running dev-server:
Module build failed: Error: Couldn't find preset "es2015"
@ohadlevy nice catch with the chunk naming. My last commit fixes that. |
Added one more fix in |
import { deprecateObjectProperty } from '../../foreman_tools'; | ||
|
||
const runningInPhantomJS = () => window._phantom !== undefined; |
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.
note we might be running also in chrome as introduced in 6044a04, perhaps you can also detect based if ENV["JS_TEST_DRIVER"] is defined.
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.
Chrom is not a problem as it implements Intl API correctly. We had issues with phantom that provides Intl object but its functionality is quite limited.
@@ -0,0 +1,42 @@ | |||
import React from 'react'; | |||
import { IntlProvider } from 'react-intl'; |
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.
does this include the functionality of Provider from 'react-redux' ? AFAIU we need it when store is not passed explicitly ?
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.
AFAIU the components/wrapperFactory.js
will wrap it with the react-redux Provider so we should be safe.
This might make it simpler: https://github.com/ratson/react-intl-redux
We can migrate later if we decide to.
|
||
/* eslint-disable react/style-prop-object */ | ||
return ( | ||
<span title={title}> |
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 means it uses the jquery popover right? perhaps we should be explicit and use the pf one?
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.
No, this means it uses standard html attribute: https://www.w3schools.com/tags/att_title.asp
I don't think jQuery is involved here.
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.
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 see. I didn't see it would apply anywhere on the dates. It's probably because the components mounted after the tooltip initialization.
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.
right, but if you want to show the correct date (e.g. a minute after the page has been loaded) you would need a different way. I"m OK to leave this outside of this pr.
'UTC', | ||
'Europe/Prague', | ||
'Europe/Kiev', | ||
'Asia/Tel_Aviv', |
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.
its Asia/Jerusalem
AFAIR :)
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.
Ok, I can change that
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.
no need, I wasn't sure if it has a meaning in terms of real timezone names or not...
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.
It worked for me. I didn't notice Asia/Tel_Aviv
tz is deemed to be deprecated, but obviously it's still available for backwards compatibility. Anyway a changed it to Jerusalem.
@@ -0,0 +1,23 @@ | |||
import React from 'react'; |
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.
hmm.. should these be part of the pr?
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.
Why not?
Notice it's living under webpack/stories/component/
and it helps to show code in the storybook.
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 needed it for the storybook. It's first time where we're using it so I think it's valid to add that. I can make it a separate commit if you want.
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 @tstrachota, I think we can merge it 👍
- it uses Asia/Jerusalem timezone - fix initial date to make it compatible with Firefox
@ohadlevy updated, stories now use |
return this.registry[name]; | ||
}, | ||
|
||
wrapperFactory() { | ||
return new WrapperFactory(); |
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.
does this happen once per document load? multiple times? I'm guess I don't understand the background for all of this change...
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.
It's called once per component mount when it's mounted into erb or angular. I added that to enable plugins using wrappers easily. An example in Katello is here:
https://github.com/Katello/katello/pull/7001/files#diff-5bcfdae0ace18ad3702fbb46115e768dR19
@ohadlevy any more comments from your side? thanks |
the last question i had is regarding the generated assets file, I currently see:
is that expected? (note the react-intl and intl - with the same language) @sharvit any idea? |
@ohadlevy - I made some research about the issue and the conclusion is the assets are fine. In details:
The implementation looks good to me, see the |
thanks for the explanation @sharvit. many thanks for your contribution @tstrachota :-) |
@tbrisker thanks! this seems to be effecting only development and related to how the hostname is being defined, looking at the console I can see:
I'm not sure where the hostname 'ohad' is used vs 'localhost' in this case, but i assume that's the reason for the failure. |
@tbrisker seems like some cases it can't figure out the |
@@ -9,13 +9,16 @@ | |||
"@storybook/addon-knobs": "^3.4.3", | |||
"@storybook/react": "^3.2.12", | |||
"@storybook/storybook-deployer": "^2.0.0", | |||
"argv-parse": "^1.0.1", |
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 missed this in the final review, but needed a packaging PR as well.
… On Wed, Jan 9, 2019 at 3:15 AM Ewoud Kohl van Wijngaarden < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In package.json
<#5184 (comment)>:
> @@ -9,13 +9,16 @@
***@***.***/addon-knobs": "^3.4.3",
***@***.***/react": "^3.2.12",
***@***.***/storybook-deployer": "^2.0.0",
+ "argv-parse": "^1.0.1",
I missed this in the final review, but needed a packaging PR as well.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5184 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABOx3DaGDKWGAmosrgvaEa2E3CvHsfyks5vBUKcgaJpZM4Rh90a>
.
|
I'm now also noticing multiple errors with this on the dashboard - https://projects.theforeman.org/issues/25812 |
Contains following changes:
react-intl
as well as the fallbackintl
package are loaded lazilyDepending on your setup, you might need to set
ASSET_PATH
env variable to get the dynamic imports (lazy loading) working in development. It's required when running over https. Eg: