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

Webpack builds! #608

Closed
wants to merge 181 commits into from
Closed

Webpack builds! #608

wants to merge 181 commits into from

Conversation

gesa
Copy link
Contributor

@gesa gesa commented Mar 18, 2020

Summary

TODO:

  • add eslint runner
  • set up dev server
  • setup a jest file transformer for html imports (webpack doesn't support the fs module, but does have an html loader)
  • create dev, prod, & shared webpack config

Checklist

  • Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

Copy link
Contributor Author

@gesa gesa left a comment

Choose a reason for hiding this comment

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

some updates on the state and some questions too

.eslintrc Outdated
"node": true
},
"globals": {
"__dirname": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think maybe we don't need this if the env object includes "node": true? will check that out.

.eslintrc Outdated
"__dirname": true
},
"rules": {
"valid-jsdoc": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

have i ever mentioned how much i love that we are sticklers with our jsdoc? bc i do.

.eslintrc Outdated
"requireReturnDescription": true
}
],
"object-curly-spacing": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

once i add prettier and have it autorun we can presumably delete this rule since it'll never come up. @crookedneighbor you good with that plan? i should add it to the next PR i do, right?

@@ -0,0 +1,14 @@
const { version } = require('../package.json');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Historically we were literally dynamically generating this file and then using fs.writeFile() which made sense at the time but maybe it's more maintainable to have a standalone conf.

package.json Outdated
"del": "^5.1.0",
"dotenv": "^8.2.0",
"es6-shim": "^0.35.5",
"eslint": "^6.8.0",
"eslint-config-braintree": "^4.0.0",
"eslint-loader": "^3.0.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

honestly i haven't decided if I want webpack to run this or if we should run it independently in posttest or the like. what it comes down to is i'd rather CI run it first, but locally run it last.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could always run it in webpack and have CI run a npm run lint task directly that is not a pre or post task for tests.

package.json Outdated
"serve-static": "^1.14.1",
"string-replace-loader": "^2.2.0",
"style-loader": "^1.1.3",
"symlink-webpack-plugin": "github:gesa/symlink-webpack-plugin#e661f2d64590a3cacba42d4f3c8a36f8d6dff1e4",
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 changes to this lib got merged in, just waiting for a new release

Copy link
Contributor

Choose a reason for hiding this comment

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

Did this ever get released?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like it https://github.com/hmsk/symlink-webpack-plugin/releases/tag/v1.0.0

Suggested change
"symlink-webpack-plugin": "github:gesa/symlink-webpack-plugin#e661f2d64590a3cacba42d4f3c8a36f8d6dff1e4",
"symlink-webpack-plugin": "^1.0.0",

src/dropin.js Outdated
@@ -734,7 +733,7 @@ Dropin.prototype._injectStylesheet = function () {

if (document.getElementById(constants.STYLESHEET_ID)) { return; }

stylesheetUrl = ASSETS_URL + '/web/dropin/' + VERSION + '/css/dropin@DOT_MIN.css';
stylesheetUrl = ASSETS_URL + '/web/dropin/' + VERSION + '/css/dropin.css';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why publish a minified and a non-minified version when we also throw all the javascript files in the npm release as-is? trying to decide how to prioritize publishing both.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify this? The intention before was to load the minified css from the cdn when using the minified version on the cdn, and the un-minified version when using the un-minified version.

I do wonder now if folks using the build from npm always got the un-minified css assets.

@@ -229,7 +229,7 @@ <h1>Braintree Drop-in demo</h1>
results.textContent = '';
error.textContent = '';

braintree.dropin.create(config, function (err, instance) {
dropin.create(config, function (err, instance) {
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 WILL BE THE DEATH OF ME. talk about a breaking change. browserify treats an option relating to how to name one's library in a parsey way—the string braintree.dropin becomes window.braintree.dropin. webpack treats that string like window['braintree.dropin']

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 may be a clever way to deal with this but if i can get away with it i'd prefer not trying to be clever.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's pretty important to keep everything name-spaced under braintree. Surely there's a way to do that, right?


module.exports = {
devServer: {
allowedHosts: ['.bt.local'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk if this is necessary, meaningful, or helpful.

}
},
output: {
library: 'dropin',
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 is that field where braintree.dropin vs dropin becomes a thing.

cgdibble and others added 27 commits May 27, 2022 10:50
* Test app accessibility fix

* Update changelog

Co-authored-by: adelavegaruiz <[email protected]>
* update braintree-web to 3.85.5

* add changelog entry

* update browser detection

* no carrot

* fix install issue

Co-authored-by: cdibble <[email protected]>
* chore:  update dependencies

* chore: add jest-environment-jsdom dependency
)

* fix: hide field errors and check for validity again on blur events

* fix: clear field error on card type change

Co-authored-by: Braintree <[email protected]>
@armandodlvr
Copy link
Contributor

closing this PR in favor of #844

@shango420
Copy link

closing this PR in favor of #844

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.