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

Build: Fix imported modules breaking ie11 #12463

Merged
merged 2 commits into from
May 24, 2019
Merged

Build: Fix imported modules breaking ie11 #12463

merged 2 commits into from
May 24, 2019

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented May 24, 2019

Fixes #12407

There were two issues:


1 - config module

Use local config module over node_modules/config:. #12403 added config dependency which is imported here:

import config from 'config';

Due to module loading here:

modules: [ 'node_modules', path.resolve( __dirname, '_inc/client' ) ],

The new node_modules/config module is used over the intended one. That introduced new ie11 errors. Fixed here by importing the module with a relative path.

We might consider ordering the module resolution so that local modules have preference.


2 - debug module

The debug module is used directly in the application:

import debugFactory from 'debug';

This dependency was not declared in package.json, so the whatever version happened to be available as a transitive dependency was used. v4 appears to be incompatible with ie11 and webpack/babel compilation and it is not used in Calypso, likely for this reason.

Fix by adding debug@3^ as a dependency.

We should enable an eslint lint rule like 'import/no-extraneous-dependencies': [ 'error', { packageDir: './' } ] to help prevent errors like this.

Testing instructions:

Try loading /wp-admin/admin.php?page=jetpack for a Jetpack site in IE11. Smoke test. The page should work properly without errors.

Proposed changelog entry for your changes:

  • Fix an issue with in ie11 with the Jetpack Dashboard.

@sirreal sirreal added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label May 24, 2019
@sirreal sirreal requested a review from a team as a code owner May 24, 2019 14:32
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello sirreal! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D28693-code before merging this PR. Thank you!

@sirreal sirreal self-assigned this May 24, 2019
@sirreal sirreal requested a review from brbrr May 24, 2019 14:33
@sirreal sirreal added [Type] Bug When a feature is broken and / or not performing as intended Admin Page React-powered dashboard under the Jetpack menu labels May 24, 2019
@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: June 4, 2019.
Scheduled code freeze: May 28, 2019

Generated by 🚫 dangerJS against 58cddc5

@sirreal sirreal changed the title Try/fix ie11 errors Build: Fix imported modules breaking ie11 May 24, 2019
@sirreal sirreal requested a review from a team May 24, 2019 15:09
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

I haven't tested in IE but code changes make sense.

debug@4 is tricky because some node_modules might start depending on it eventually.

@jeherve jeherve added this to the 7.4 milestone May 24, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

My IE11 VM won't start anymore, so I could only test in a different browser. I can confirm that the dashboard is displayed nicely and analytics still work.

I'll let someone else test in IE11.

Copy link
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

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

Dashboard works fine now!

There is quite a problem with "My Plan" page though, which I think is not related to this PR.

@brbrr brbrr added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 24, 2019
@jeherve
Copy link
Member

jeherve commented May 24, 2019

There is quite a problem with "My Plan" page though, which I think is not related to this PR.

Logged in #12464

@jeherve jeherve merged commit b0e5296 into master May 24, 2019
@jeherve jeherve deleted the try/fix-ie11-errors branch May 24, 2019 18:41
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels May 24, 2019
jeherve added a commit that referenced this pull request May 24, 2019
jeherve added a commit that referenced this pull request May 27, 2019
* Kick off the changelog

* Add 7.3.1

* Update date and post link

* changelog: add #12219

* changelog: add #12170

* changelog: add #12184

* Changelog: add #12268

* Changelog: add #12081

* Changelog: add #12323

* Changelog: add #12204

* Changelog: add #12269

* Changelog: add #12332

* changelog: add #12339

* changelog: add #12209

* Changelog: add #12319

* Changelog: add #12357

* Changelog: add #12124

* Changelog: add #12373

* Changelog: add #12252

* Changelog: add #12383

* Changelog: add #12372

* changelog: add #12337

* Changelog: add #12290

* Changelog: add #12301

* Changelog: add #12061

* Testing list: add instructions for #12061

* Changelog: add #12393

* Update minimum supported version

See #12287

* Changelog: add #12406

* Testing list: add #12406

* Changelog: add #12277

* Changelog: add #12412

* Changelog: add #11318

* Changelog: add #12328

* Changelog: add #12425

* Changelog: add #12380

* Changelog: add #12428

* Changelog: add #12414

* Changelog: add #12395

* Changelog & Testing list: add #12416, #12417, #12418, and #12348

* changelog: add #12379

* Changelog: add #12341

* changelog: add #12444

* Changelog: add #12434

* Changelog: add #12454

* Changelog: add #12460

* Changelog: add #12463

* Changelog: add #12457

* Changelog / testing list: add #10333

* Changelog: add #12467


Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
anomiex added a commit that referenced this pull request Nov 12, 2021
There's no point to it, the generated comments just bloat the
CSS and license.txt files.

Note Webpack already defaults it to true for `mode: development` builds,
where it makes a little more sense for the comments to be present.

It's not clear why it was ever being set in the first place,
neither #12463 nor #13070 seem to have any discussion about the
additions.
anomiex added a commit that referenced this pull request Nov 12, 2021
…21727)

There's no point to it, the generated comments just bloat the
CSS and license.txt files.

Note Webpack already defaults it to true for `mode: development` builds,
where it makes a little more sense for the comments to be present.

It's not clear why it was ever being set in the first place,
neither #12463 nor #13070 seem to have any discussion about the
additions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard: Not Displaying in IE 11
6 participants