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

chore(cli app scripts): update to 5.7.2 and adjust related files #523

Merged
merged 10 commits into from
Mar 5, 2021

Conversation

Mohammer5
Copy link
Contributor

@Mohammer5 Mohammer5 commented Feb 25, 2021

  • Upgraded the @dhis2/cli-app-scripts package to 5.7.0
  • Changed build/[es|cjs]/lib.js to ./build/[es|cjs]/index.js
  • Added the exports field to all workspaces' `package.json
  • Added locale files to sideeffects in the widgets and forms workspaces
  • Adjusted the storybook's babel config file
  • Figure out if the changes to get the icons library to work again are legit

When running yarn build:icons on master, the following files are being created:

packages/icons/build/es/lib.js
packages/icons/build/cjs/lib.js

When switching to this branch, this does not happen anymore until I added an index.js to the icons' src folder which imports ./react/index.js

@Mohammer5 Mohammer5 requested review from varl and amcgee February 25, 2021 16:32
@Mohammer5 Mohammer5 changed the title Libs 129 chore(cli app scripts): update to 5.7.0 and adjust related files Feb 25, 2021
@cypress
Copy link

cypress bot commented Feb 25, 2021



Test summary

502 0 0 0


Run details

Project ui
Status Passed
Commit b77d6c3
Started Mar 5, 2021 2:00 PM
Ended Mar 5, 2021 2:04 PM
Duration 04:09 💡
OS Linux Ubuntu - 20.04
Browser Electron 87

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

Code change LGTM, obviously tests need to pass and I haven't tried running this locally

packages/icons/package.json Outdated Show resolved Hide resolved
@varl varl changed the title chore(cli app scripts): update to 5.7.0 and adjust related files chore(cli app scripts): update to 5.7.2 and adjust related files Feb 26, 2021
@varl
Copy link
Contributor

varl commented Feb 26, 2021

There is a funky test failure in the Transfer's LeftSide component when it imports the background color. I can resolve it manually, so it's definitely there, but for some reason loading the CJS fails in the test.

@varl
Copy link
Contributor

varl commented Feb 26, 2021

Ah, I see:

./core/src/AlertBar/AlertBar.stories.js: Error: styled-jsx/css: if you are getting this error it means that your css tagged template literals were not transpiled.

@Mohammer5
Copy link
Contributor Author

There is a funky test failure in the Transfer's LeftSide component when it imports the background color. I can resolve it manually, so it's definitely there, but for some reason loading the CJS fails in the test.

I've seen that one as well. But I can't figure out why it's happening. It's never failed before, the imports seem correct. I was able to get the test to succeed again by renaming that file, run the test and then rename the file back to its original name. The second time the error occurred, I wasn't able to do anything about it (even after deleting all node module folders, clearing jest's and yarn's caches and then installing the packages again)

@varl

This comment has been minimized.

@varl
Copy link
Contributor

varl commented Feb 26, 2021

The test is failing because we now copy all the source files to the build directory, and Jest is very eager when it comes to finding and running tests, so it also runs them from the build/ directory. The tests don't necessarily work in their transpiled variants, so we get problems.

This fixes the test command in app-scripts: dhis2/app-platform#521

@varl
Copy link
Contributor

varl commented Mar 5, 2021

There is some strange interactions going on between Storybook's Webpack and Bable setup, where some sources are imported through the entry-points in the build folder, and some are imported through the src folder. The build imports are already transpiled so they work properly, but the src imports somehow end up not being transpiled by Storybook's Webpack+Bable combo, even after overriding them.

When I swap out e.g. import { Button } from './Button.js' to import { Button } from '@dhis2/ui' in Button.stories.js, Storybook starts working correctly.

@Mohammer5
Copy link
Contributor Author

Mohammer5 commented Mar 5, 2021

When I swap out e.g. import { Button } from './Button.js' to import { Button } from '@dhis2/ui' in Button.stories.js, Storybook starts working correctly.

In my opinion importing from the built library is the actual correct implementation, because it doesn't require that storybook needs to be aware of any build steps required to get the component to work.
On the other side I can see why it's handy to just import the component via the relative path..
Given the current situation, I think I'd prefer changing the imports in all stories, if you agree (importing from '@dhis2/...')

EDIT: Good find, btw

@varl
Copy link
Contributor

varl commented Mar 5, 2021

@Mohammer5 That brings us to the next step.

Nothing in subpackages is allowed to import @dhis2/ui. We can either ESLint ignore in every Storybook file, or move the Storybook stuff outside of core.

@varl
Copy link
Contributor

varl commented Mar 5, 2021

Alright, I figured out a way to override the Storybook Babel configuration with our necessary customization.

@varl varl enabled auto-merge March 5, 2021 14:09
@varl varl merged commit 24036ca into master Mar 5, 2021
@varl varl deleted the LIBS-129 branch March 5, 2021 14:19
@dhis2-bot
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants