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

Add build task for publishing #266

Merged
merged 23 commits into from Dec 2, 2018
Merged

Add build task for publishing #266

merged 23 commits into from Dec 2, 2018

Conversation

elanoism
Copy link
Contributor

@elanoism elanoism commented Aug 6, 2018

Resolves #109.

The goal for this is to make circuit-ui easy to add to a project without additional modifications. This is most important for apps that are built with CRA where you cannot change the config.

Changes

  • Change .babelrc so we have different configs for different env targets.
  • Add a build command that transpiles the code to both cjs and es modules and puts them in lib.

I've created two example repos using this.

For testing, you need circuit-ui and the example app working side by side and:

  • Run yarn build:js in the circuit-ui folder
  • Run yarn upgrade circuit-ui in the example folder

@elanoism elanoism changed the title Add build task for publishing the lib Add build task for publishing Aug 6, 2018
Copy link
Collaborator

@felixjung felixjung left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Please check my comment about the babel preset.

package.json Outdated
"babel-plugin-module-resolver": "^3.0.0",
"babel-preset-es2015": "^6.24.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why you're not using the env preset? AFAIK, all the ES version specific presets are deprecated.

Copy link
Contributor

@fernandofleury fernandofleury left a comment

Choose a reason for hiding this comment

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

🙇 neat stuff. We just need to check the preset-env instead of 2015

@mlent
Copy link
Contributor

mlent commented Aug 7, 2018

I'm not really the build expert to really tell you if this is the right way to go, but my impression is that it looks great and will allow us to move forward.

Thanks a ton for the extra initiative and focusing on a simple solution 👍

Once you address Felix/Fernando's feedback item, you have my approval!

@connor-baer
Copy link
Member

connor-baer commented Aug 7, 2018

@elanoism Just a note that one needs to have coreutils installed to copy the *.svg files with gcp. Can be done with brew install coreutils, maybe add a comment about it?

Copy link
Member

@connor-baer connor-baer left a comment

Choose a reason for hiding this comment

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

I published this PR to npm under the canary tag as version 0.0.1-canary, then I took it for a spin with Next.js and Webpack 4 on a test project. The treeshaking works great, I'd estimate that it'll reduce the website's bundle size by 1.5MB (or 250kB gzipped). 🙌

However, there's one thing we need to fix:

Importing from @sumup/circuit-ui works, but importing from a subdirectory such as @sumup/circuit-ui/themes doesn't. Instead, it would need to be @sumup/circuit-ui/lib/themes — tedious to type and not backward compatible.

Suggestion: replicate @felixjung's approach and copy package.json, LICENSE.md, README.md, and any other required files to the dist folder, so we can publish from there with a flat file structure.

I'm happy to make these changes myself if you agree. :)

package.json Outdated Show resolved Hide resolved
@elanoism
Copy link
Contributor Author

elanoism commented Aug 8, 2018

Replaced preset-es2015 with preset-env. We should just make sure not to add usage options to it, because it will start adding polyfills and fatten the bundle. This is not what we want :)

Gathered some feedback locally and the theme is now exported trough the main export as:

import { theme } from 'circuit-ui';
<ThemeProvider theme={theme.standard}>...</Theme>

This comes from @connor-baer and leaves the possibility open for adding dark, light in the future.
WDYT? @felixjung @mlent

Generally we should try to export everything trough the main export in order to leverage bundlers choosing appropriate module type.

Later I will focus on exporting the utils trough the main export as well.

@mlent
Copy link
Contributor

mlent commented Aug 8, 2018

@elanoism As long as you spell standard correctly :)

@elanoism
Copy link
Contributor Author

elanoism commented Aug 8, 2018

I rebased it in so .... it didn't happen.

mlent
mlent previously approved these changes Aug 8, 2018
Copy link
Contributor

@mlent mlent left a comment

Choose a reason for hiding this comment

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

Looks good to me :) Thanks for going the extra mile on this.

@connor-baer
Copy link
Member

@elanoism Will you also export the style helpers at root level?

connor-baer
connor-baer previously approved these changes Aug 9, 2018
@elanoism
Copy link
Contributor Author

elanoism commented Aug 9, 2018

Added the utils export.

Rebased on master and made a release @sumup/circuit-ui@0.0.3-canary

@connor-baer . @herberthenrique , please try it out.

I need to move on to different things, before I can spin it up in the dashboard, but we first need to update all places where we use it, before we can merge this safely to master.

P.S. Take a look at the exports in the bottom for styles/globalStyles/utils and stuff.

@connor-baer connor-baer added the ⛔ do not merge There is a blocker label Aug 9, 2018
@elanoism elanoism added this to the v1.0.0 milestone Aug 9, 2018
Copy link
Contributor

@herberthenrique herberthenrique left a comment

Choose a reason for hiding this comment

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

I had updated Partner Portal to use this canary version and works perfectly <3

@connor-baer
Copy link
Member

connor-baer commented Aug 10, 2018

@elanoism I upgraded the website to Webpack 4 (adds support for sideEffects: false) and installed the canary version of Circuit UI. It works great apart from a few warning messages from Webpack:

 WARNING  Compiled with 4 warnings                      23:30:25
 warning  in ./node_modules/@sumup/circuit-ui/lib/es/index.js

"export 'MessageButton' was not found in './components/Button'

 warning  in ./node_modules/@sumup/circuit-ui/lib/es/index.js

"export 'MessageIcon' was not found in './components/Button'

 warning  in ./node_modules/@sumup/circuit-ui/lib/es/components/
CreditCardDetails/components/ExpiryDateInput/index.js

"export 'parseExpiryDate' was not found in './ExpiryDateInputSer
vice'

 warning  in ./node_modules/@sumup/circuit-ui/lib/es/components/
CreditCardDetails/components/SecurityCodeInput/index.js

"export 'parseSecurityCode' was not found in './SecurityCodeInpu
tService'

I didn't look into the errors any further.

Also, the reduction in bundle size won't be as big as I estimated. I'm seeing improvements of 400kb (110kb gzipped). Still substantial though!

@mlent mlent mentioned this pull request Aug 11, 2018
7 tasks
@elanoism
Copy link
Contributor Author

elanoism commented Aug 13, 2018

@connor-baer , these are general bugs in the library. I can fix them before release, though.

Edit: @herberthenrique is addressing those in #279. I will publish a new npm version after it is merged.

@mlent
Copy link
Contributor

mlent commented Aug 20, 2018

@elanoism @felixjung @fernandofleury Looks like there are a lot of approvals, is there more work to be done before this can be merged? 🎉

@elanoism
Copy link
Contributor Author

Published next release 0.0.4-canary including the latest changes from master.

@mlent, we need to prepare extra release for the dashboard (website and referrals are already fine, afaik) before we can merge this. We believe that due to the name change it may cause problems otherwise. I can do it after the release of the transactions history.

@codecov
Copy link

codecov bot commented Nov 28, 2018

Codecov Report

Merging #266 into master will increase coverage by 0.23%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
+ Coverage   66.71%   66.95%   +0.23%     
==========================================
  Files         135      135              
  Lines        1992     1985       -7     
  Branches      321      319       -2     
==========================================
  Hits         1329     1329              
+ Misses        541      536       -5     
+ Partials      122      120       -2
Impacted Files Coverage Δ
src/util/ownerWindow.js 0% <0%> (ø) ⬆️
src/components/SideNav/components/Modal/Modal.js 0% <0%> (ø) ⬆️
src/components/SideNav/components/Drawer/Drawer.js 0% <0%> (ø) ⬆️
src/util/helpers.js 0% <0%> (ø) ⬆️
...editCardDetails/CreditCardDetails.disabledStory.js 0% <0%> (ø) ⬆️
src/components/SideNav/transitions.js 0% <0%> (ø) ⬆️
...components/SideNav/components/Backdrop/Backdrop.js 0% <0%> (ø) ⬆️
...omponents/SideNav/components/Modal/ModalManager.js 0% <0%> (ø) ⬆️
src/components/SideNav/components/List/List.js 0% <0%> (ø) ⬆️
src/styles/global-styles.js 0% <0%> (ø) ⬆️
... and 4 more

connor-baer
connor-baer previously approved these changes Nov 28, 2018
mlent
mlent previously approved these changes Nov 28, 2018
@connor-baer connor-baer added 🛠️ tech Changes to the tech stack or infrastructure ready for review and removed ⛔ do not merge There is a blocker labels Nov 28, 2018
fernandofleury
fernandofleury previously approved these changes Nov 29, 2018
Copy link
Contributor

@fernandofleury fernandofleury left a comment

Choose a reason for hiding this comment

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

Maybe also update the readme with the new commands for releasing?

@connor-baer
Copy link
Member

@elanoism Ready to merge? 🚀

@elanoism elanoism merged commit 237d480 into master Dec 2, 2018
@elanoism elanoism deleted the tech/experimental-build branch December 2, 2018 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠️ tech Changes to the tech stack or infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants