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

dom4 v2 + updated polyfills #2043

Merged
merged 12 commits into from
Jan 26, 2018
Merged

dom4 v2 + updated polyfills #2043

merged 12 commits into from
Jan 26, 2018

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Jan 25, 2018

Fixes #2032

Changes proposed in this pull request:

  • 🔥 Blueprint requires Array.from and Array.fill ES2015 polyfills, in addition to already required Map/Set
    • good news: all four symbols are in the "iterator" group so they'll typically get polyfilled together (they're all in the same typescript lib file)
  • bump dom4 to v2.0
    • query => querySelector
    • queryAll => Array.from(querySelectorAll)
  • ⭐️ replace es6-shim with minimal core-js submodules
  • test-commons/bootstrap module imports polyfills and configures enzyme. imported as first line of all karma suites.

@blueprint-bot
Copy link

remove table index es6-shim import

Preview: documentation | landing | table


- `Map`
- `Set`
- `Array.from`
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -4,8 +4,6 @@
* Licensed under the terms of the LICENSE file distributed with this project.
*/

import "es6-shim";
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to add polyfills to docs-app and landing-app. I suggest core-js submodules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

truth. we're using es6-shim in devDeps in every package for tests. should I switch wholesale over to core-js?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, switch wholesale

@blueprint-bot
Copy link

[PoC] use core-js instead of es6-shim, test-commons polyfills

Preview: documentation | landing | table

@blueprint-bot
Copy link

applyPolyfills() function

Preview: documentation | landing | table

import * as Enzyme from "enzyme";
import * as Adapter from "enzyme-adapter-react-16";

Enzyme.configure({ adapter: new Adapter() });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adidahiya thoughts?

*/

import "./lib/cjs/polyfill";
import "./lib/cjs/enzymeConfigure"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this js file in root rather than reaching into /lib/cjs/

@blueprint-bot
Copy link

use test-commons/bootstrap in all test suites, remove es6-shim entirely

Preview: documentation | landing | table

@giladgray giladgray changed the title dom4 v2 + Array.from requirement dom4 v2 + updated polyfills Jan 25, 2018
@blueprint-bot
Copy link

Array.fill polyfill required for table

Preview: documentation | landing | table

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

lgtm, just small comments

@@ -39,9 +39,6 @@
"@blueprintjs/karma-build-scripts": "^0.4.0",
"@blueprintjs/node-build-scripts": "^0.4.0",
"@blueprintjs/test-commons": "^0.4.0",
"enzyme": "^3.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

you still need enzyme here to adhere to no-implicit-dependencies (which we need to turn on at some point)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just enzyme, right? no adapter?

@@ -0,0 +1,9 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

just inline this into bootstrap.js

* Require the minimal set of ES2015+ polyfills from `core-js` library.
* See "NPM Installation" section of docs homepage for more information.
*/
import "core-js/fn/array/fill";
Copy link
Contributor

Choose a reason for hiding this comment

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

make this a JS file. no point in compiling this with tsc

@blueprint-bot
Copy link

bring back enzyme dep

Preview: documentation | landing | table

@adidahiya adidahiya merged commit 22a1f0c into develop Jan 26, 2018
@adidahiya adidahiya deleted the gg/dom4 branch January 26, 2018 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants