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

Update to Webpack 4 and fix a lot of things #145

Merged
merged 17 commits into from
Apr 22, 2019

Conversation

somewhatabstract
Copy link
Contributor

@somewhatabstract somewhatabstract commented Apr 20, 2019

This addresses #141, #126, and #125.

It upgrades us to Webpack 4 and all the latest loaders and related things. Also updates handlebars and a couple of other things to address some newer security audit issues.

Also adds some new npm scripts to make development easier and splits dev and prod builds properly. It also removes build assets from the repo (we'll have to update the Github pages accordingly) and gets us ready to start publishing to npm.

Complete list of changes in the commits.

.arcconfig
.arclint
.travis.yml
.eslintrc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't see much point in packaging these sorts of things.

"test",
"tests"
]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bower is dead and I think keeping this around is only going to leave it going stale

devServer: {
open: true,
openPage: "test"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bit is new. It opens our test page when the dev server launches.

veryimportant,
autoprefixer({browsers: ["> 1%"]}),
plugins: [
new CleanWebpackPlugin(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bit is new.

plugins,
postcss: [
veryimportant,
autoprefixer({browsers: ["> 1%"]}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to plugins.

@somewhatabstract somewhatabstract changed the title Update readme to cover some of the changes Update to Webpack 4 and fix a lot of things Apr 20, 2019
@somewhatabstract somewhatabstract self-assigned this Apr 20, 2019
Copy link

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

This all looks quite reasonable to me. Thanks for removing the dist folder from the repo and updating the webpack config.

@@ -13,7 +13,7 @@ const plugins = require("./plugins");
const logoTemplate = require("./templates/logo.handlebars");

// Chrome Accessibility Developer Tools - required once as a global
require("script!./node_modules/accessibility-developer-tools/dist/js/axs_testing.js");
require("script-loader!./node_modules/accessibility-developer-tools/dist/js/axs_testing.js");

Choose a reason for hiding this comment

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

That's annoying that they don't have a UMD version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There might be a better way, now. But that'll have to be a different change. :)

"name": "tota11y",
"version": "0.1.6",
"name": "@khanacademy/tota11y",
"version": "0.2.0",

Choose a reason for hiding this comment

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

yay!

package.json Outdated
"lint": "eslint index.js plugins test utils",
"test": "mocha --require test/babel-hook test/*.js",
"live-test": "webpack-dev-server --config webpack.config.babel.js --hot --inline"
"test:live": "webpack-dev-server --config webpack.config.babel.js --hot --inline",
"start": "npm run test:live",

Choose a reason for hiding this comment

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

What's the use of test:live if it's the same thing as start?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point!

"repository": {
"type": "git",
"url": "https://github.com/Khan/tota11y.git"
},
"author": "Jordan Scales <scalesjordan@gmail.com>",
"contributors": [
"Jeff Yates <jeff.yates@alumni.manchester.ac.uk"

Choose a reason for hiding this comment

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

This is cool. I think I'll suggest this for KaTeX as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think it should be voluntary (hence why I haven't populated it with all the folks that have had changes merged), but we should ask folks if they want to be in this list.

<script src="http://localhost:8080/webpack-dev-server.js"></script>
<script src="../tota11y.min.js"></script>
<script src="/webpack-dev-server.js"></script>
<script src="/tota11y.js"></script>

Choose a reason for hiding this comment

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

nice!

// Make the JSX pragma function available everywhere without the need
// to use "require"
new webpack.ProvidePlugin({
[options.jsxPragma]: path.join(__dirname, "utils", "element"),

Choose a reason for hiding this comment

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

I'm a little unclear exactly what this is doing, but this is what we were doing before so I guess it's alright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that this allows us to use JSX without React. We inject our own calls to support creating elements so that when JSX is compiled to JS, it will work.

@somewhatabstract somewhatabstract merged commit a490500 into master Apr 22, 2019
@somewhatabstract somewhatabstract deleted the upgradehandlebarsagain branch April 22, 2019 17:08
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.

2 participants