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

[video-react/video-react#237] Support non-Rollup/ES6-based toolchains #242

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@jeremyckahn
Copy link

jeremyckahn commented Mar 11, 2019

This fixes the issues I've been having (please see #237) and I hope that it fixes import problems for others. This seems to be the preferred way to publish modules per Rollup's documentation.

I may have something wrong here in terms of supporting non-Webpack environments like Rollup, as I am not really familiar with that tool!

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 11, 2019

Codecov Report

Merging #242 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #242   +/-   ##
=======================================
  Coverage   45.08%   45.08%           
=======================================
  Files          46       46           
  Lines        1211     1211           
=======================================
  Hits          546      546           
  Misses        665      665

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 38a35c2...4a56536. Read the comment docs.

@mondaychen

This comment has been minimized.

Copy link
Member

mondaychen commented Mar 12, 2019

We have been following reactstrap's build script: use rollup to build bundled output in dist and use babel to generate files with one-to-one mapping in lib.

I think our current setup helps build tools generate better source maps, since it keeps the original file/folder structure. If you use the bundled file in dist, it's gonna be one giant source file. (Or maybe some newer build tools are able to use the sourcemap files in dist?)

cc @JimLiu @xiaoyuhen

@jeremyckahn

This comment has been minimized.

Copy link
Author

jeremyckahn commented Mar 12, 2019

The way that files are generated seems fine to me, the main field is just referencing a file that can't be used by downstream projects without having to install unrelated Babel dependencies. It's my understanding that the package.json's main field should reference as file that's as universally compatible project as possible, and currently lib/index.js begins with these lines:

"use strict";

var _interopRequireWildcard = require("@babel/runtime/helpers/interopRequireWildcard");

var _interopRequireDefault = require("@babel/runtime/helpers/interopRequireDefault");

This breaks downstream projects that do not have @babel/runtime installed.

Thinking about this more, I may have described the problem this PR solves somewhat incorrectly, so I'll restate it here: The issue is that the current main field assumes certain Babel dependencies are installed, which breaks projects that don't have those dependencies.

@mondaychen

This comment has been minimized.

Copy link
Member

mondaychen commented Mar 12, 2019

Thank you for your detailed reply. Now I understand the problem better.

Based on my investigation, adding "@babel/runtime": "^7.2.0" to dependencies of package.json should fix this problem too. This problem was caused by introducing @babel/plugin-transform-runtime but not the necessary dependency @babel/runtime.

https://babeljs.io/docs/en/babel-plugin-transform-runtime

It's also in reactstrap's dependencies too: https://github.com/reactstrap/reactstrap/blob/master/package.json

Can you give this solution a try and let me know if it works?

@jeremyckahn

This comment has been minimized.

Copy link
Author

jeremyckahn commented Mar 13, 2019

Yes that sounds like the proper fix to me! Thank you for looking into this. I won't be able to test this until I am back at work tomorrow, but I think your approach is the correct one. 😄

@jeremyckahn

This comment has been minimized.

Copy link
Author

jeremyckahn commented Mar 13, 2019

Update: This is the correct fix: #245

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.