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

Associate version to entry files #1060

Merged
merged 6 commits into from
Sep 16, 2016

Conversation

vera-liu
Copy link
Contributor

@vera-liu vera-liu commented Sep 2, 2016

Not sure if I did the right steps for testing:

  1. Make changes in js file, update the version number in package.json
  2. npm install under assets
  3. npm run dev (without this step the new js files are not generated by webpack, is this right?)
  4. python setup.py
  5. caravel db upgrade
  6. rerun caravel server
  7. Changes reflect in page on non-hard refresh

@mistercrunch @ascott @bkyryliuk

@@ -7,6 +7,11 @@ const APP_DIR = path.resolve(__dirname, './');
// output dir
const BUILD_DIR = path.resolve(__dirname, './dist');

const fs = require('fs');
const contents = fs.readFileSync('package.json');
const JSONObject = JSON.parse(contents);
Copy link
Contributor Author

@vera-liu vera-liu Sep 2, 2016

Choose a reason for hiding this comment

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

Somehow this part did not re-run when js file/package.json is changed, not sure when exactly does webpack reload its config.js file

Copy link
Member

Choose a reason for hiding this comment

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

I think it's only expected to run when an npm run (...) command is executed

@vera-liu vera-liu force-pushed the vliu_add_version_to_entry branch from b50208a to a043a34 Compare September 2, 2016 17:05
@@ -19,7 +24,7 @@ const config = {
},
output: {
path: BUILD_DIR,
filename: '[name].entry.js',
filename: '[name].' + VERSION_STRING + '.entry.js',
Copy link
Member

@mistercrunch mistercrunch Sep 6, 2016

Choose a reason for hiding this comment

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

optional nit, equivalent to :

`[name].${VERSION_STRING}.entry.js`

Copy link

Choose a reason for hiding this comment

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

@mistercrunch
Copy link
Member

A few minor nits, but otherwise LGTM. Oh and please fix the Code Climate issue!

@@ -7,6 +7,11 @@ const APP_DIR = path.resolve(__dirname, './');
// output dir
const BUILD_DIR = path.resolve(__dirname, './dist');

const fs = require('fs');
Copy link

Choose a reason for hiding this comment

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

let's put all our require statements at the top of the file

@ascott
Copy link

ascott commented Sep 6, 2016

should package.json version be the same as caravel version?

using this method we would need to remember to update the package.json version with any js change. this would invalidate the cache on ALL of the js files and not just on the files that changed. not quite what we want to do here.

essentially, we want the filename to update if the file changed, and to stay the same if the contents did not change. we should do this for the css files as well as the js files. i don't think we should need to pull the version from python. we should be able to achieve this using webpack and hashes.

here's a good article on how to do this using webpack: https://medium.com/@okonetchnikov/long-term-caching-of-static-assets-with-webpack-1ecb139adb95

@ascott ascott added airbnb Airbnb related ui and removed help wanted labels Sep 7, 2016
@vera-liu vera-liu force-pushed the vliu_add_version_to_entry branch 3 times, most recently from ef493e8 to 8f883b5 Compare September 7, 2016 20:23
@vera-liu vera-liu force-pushed the vliu_add_version_to_entry branch from 8f883b5 to 76b9bc5 Compare September 7, 2016 21:02
@mistercrunch
Copy link
Member

LGTM

@ascott
Copy link

ascott commented Sep 15, 2016

nice work! 👍

@vera-liu vera-liu merged commit 2432c31 into apache:master Sep 16, 2016
dennisobrien pushed a commit to dennisobrien/caravel that referenced this pull request Sep 19, 2016
* Associate version to entry files

* Modified path joins for configs

* Made changes based on comments
vera-liu added a commit to vera-liu/caravel that referenced this pull request Sep 27, 2016
* Associate version to entry files

* Modified path joins for configs

* Made changes based on comments
vera-liu added a commit that referenced this pull request Sep 29, 2016
…1205)

* create structure for new forked explore view (#1099)

* create structure for new forked explore view

* update component name

* add bootstrap data pattern

* remove console.log

* Associate version to entry files (#1060)

* Associate version to entry files

* Modified path joins for configs

* Made changes based on comments

* Created store and reducers (#1108)

* Created store and reducers

* Added spec

* Modifications based on comments

* Explore control panel components: Chart control, Time filter, SQL,
GroupBy and Filters

* Modifications based on comments
ascott pushed a commit that referenced this pull request Oct 4, 2016
* Explore control panel - Chart control, TimeFilter, GroupBy, Filters (#1205)

* create structure for new forked explore view (#1099)

* create structure for new forked explore view

* update component name

* add bootstrap data pattern

* remove console.log

* Associate version to entry files (#1060)

* Associate version to entry files

* Modified path joins for configs

* Made changes based on comments

* Created store and reducers (#1108)

* Created store and reducers

* Added spec

* Modifications based on comments

* Explore control panel components: Chart control, Time filter, SQL,
GroupBy and Filters

* Modifications based on comments

* accommodate old and new explore urls

* move bootstrap data up in scope

* fix code climate issues

* fix long lines

* fix syntax error
vera-liu added a commit to vera-liu/caravel that referenced this pull request Oct 4, 2016
…pache#1205)

* create structure for new forked explore view (apache#1099)

* create structure for new forked explore view

* update component name

* add bootstrap data pattern

* remove console.log

* Associate version to entry files (apache#1060)

* Associate version to entry files

* Modified path joins for configs

* Made changes based on comments

* Created store and reducers (apache#1108)

* Created store and reducers

* Added spec

* Modifications based on comments

* Explore control panel components: Chart control, Time filter, SQL,
GroupBy and Filters

* Modifications based on comments
vera-liu added a commit that referenced this pull request Oct 4, 2016
* Explore control panel - Chart control, TimeFilter, GroupBy, Filters (#1205)

* create structure for new forked explore view (#1099)

* create structure for new forked explore view

* update component name

* add bootstrap data pattern

* remove console.log

* Associate version to entry files (#1060)

* Associate version to entry files

* Modified path joins for configs

* Made changes based on comments

* Created store and reducers (#1108)

* Created store and reducers

* Added spec

* Modifications based on comments

* Explore control panel components: Chart control, Time filter, SQL,
GroupBy and Filters

* Modifications based on comments

* Added access check + Druid in endpoint

* pull grains to constants

* Switch explore.html to old version
vera-liu added a commit to vera-liu/caravel that referenced this pull request Oct 4, 2016
…pache#1205)

* create structure for new forked explore view (apache#1099)

* create structure for new forked explore view

* update component name

* add bootstrap data pattern

* remove console.log

* Associate version to entry files (apache#1060)

* Associate version to entry files

* Modified path joins for configs

* Made changes based on comments

* Created store and reducers (apache#1108)

* Created store and reducers

* Added spec

* Modifications based on comments

* Explore control panel components: Chart control, Time filter, SQL,
GroupBy and Filters

* Modifications based on comments
@vera-liu vera-liu deleted the vliu_add_version_to_entry branch November 1, 2016 19:03
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
airbnb Airbnb related 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants