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

Publish package sources along with transpiled files #2604

Merged
merged 2 commits into from
Dec 30, 2017
Merged

Conversation

Hypnosphi
Copy link
Member

Issue: #2602

In #2589, I introduced jsnext:main fields so that eslint can resolve named and default exports when importing one package from another. They point to files in src directories that aren't published currently.

@Hypnosphi Hypnosphi added bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Dec 28, 2017
@codecov
Copy link

codecov bot commented Dec 28, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2604   +/-   ##
=======================================
  Coverage   32.61%   32.61%           
=======================================
  Files         397      397           
  Lines        8855     8855           
  Branches      951      937   -14     
=======================================
  Hits         2888     2888           
- Misses       5317     5327   +10     
+ Partials      650      640   -10
Impacted Files Coverage Δ
app/react/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/react/src/server/utils.js 0% <0%> (-53.58%) ⬇️
lib/ui/src/libs/key_events.js 40% <0%> (ø) ⬆️
addons/events/src/components/Event.js 38.55% <0%> (ø) ⬆️
addons/a11y/src/components/Report/Info.js 0% <0%> (ø) ⬆️
app/react-native/src/preview/story_kind.js 0% <0%> (ø) ⬆️
addons/info/src/components/types/Enum.js 0% <0%> (ø) ⬆️
addons/a11y/src/components/Report/index.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/api/configs/init_api.js 40.42% <0%> (ø) ⬆️
.../src/modules/ui/components/stories_panel/header.js 29.62% <0%> (ø) ⬆️
... and 43 more

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 73ef602...f84f876. Read the comment docs.

@igor-dv
Copy link
Member

igor-dv commented Dec 29, 2017

I think we don't want to publish src files...

@marcin-mazurek
Copy link

marcin-mazurek commented Dec 29, 2017

@igor-dv If you don't publish the src folder, then it doesn't make sense to set the esnext:main to src :)

@Hypnosphi
Copy link
Member Author

I think we don't want to publish src files

What's exactly the problem with it? I find it very convenient to be able to view external packages' sources directly in node_modules directory

@tmeasday
Copy link
Member

I don't feel very expert on this but I've seen it done before and it feels OK to me.

@Hypnosphi Hypnosphi requested a review from a team December 30, 2017 03:48
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Happy to approve this

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

This would allow smart bundlers to tree-shake, correct?

LGTM

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Dec 30, 2017

This would allow smart bundlers to tree-shake

Well we can actually to that ourselves, by tweaking our default webpack config =)
And I believe we're only ones who bundle our packages (so it raises the question, why even transpile before publishing)

@Hypnosphi Hypnosphi merged commit 9751076 into master Dec 30, 2017
@Hypnosphi Hypnosphi deleted the publish-src branch December 30, 2017 12:24
shilman pushed a commit that referenced this pull request Jan 7, 2018
Publish package sources along with transpiled files
@Hypnosphi Hypnosphi added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants