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

Support non-ES modules #104

Merged
merged 1 commit into from
Jun 30, 2018

Conversation

sergeysolovev
Copy link
Contributor

To fix #103

HOC loadable can be used for any modules that webpack can import. I’ve tested it on my project and it works fine for both ES modules and modules containing markdown text.

@codecov
Copy link

codecov bot commented Jun 23, 2018

Codecov Report

Merging #104 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
- Coverage    90.5%   90.45%   -0.05%     
==========================================
  Files          15       15              
  Lines         200      199       -1     
  Branches       54       54              
==========================================
- Hits          181      180       -1     
  Misses         17       17              
  Partials        2        2
Impacted Files Coverage Δ
src/utils/resolveModuleDefault.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8402c19...dfd4b4c. Read the comment docs.

/* eslint-disable no-underscore-dangle */
const resolveModuleDefault = module =>
module.__esModule ? module.default : module
const resolveModuleDefault = module => module.default || module
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the real problem - .default for non ES module, or not .default for ES module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the real problem - .default for non ES module, or not .default for ES module?

.default for non ES module.

The problem is inconsistency between dynamic imports of webpack 4 vs. babel-plugin-dynamic-import-node. This is a known issue and currently there are 2 workarounds. Both of them are described here: airbnb/babel-plugin-dynamic-import-node#47.

This PR replaces one of these workarounds (deleted code) with the other one (added code). As a result, loadable-components will have better support for webpack 4.

Copy link
Collaborator

Choose a reason for hiding this comment

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

React-loadable and universal-component are both looking for _esModule
Async and Imported component are looking only for .default

@neoziro - it's your turn.

@gregberge gregberge merged commit 2a82314 into gregberge:master Jun 30, 2018
@gregberge
Copy link
Owner

@sergeysolovev thanks for the fix. I think non ES modules exposing a .default key in their default exports are very very uncommon especially in code splitting use-cases. So I consider this change like a compatibility improvement rather than a breaking change.

@sergeysolovev
Copy link
Contributor Author

@neoziro thank you for the great lib!

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.

Add support for any webpack assets
3 participants