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

STRWEB-69: Add support for transpilation #73

Merged
merged 47 commits into from
Mar 21, 2023
Merged

STRWEB-69: Add support for transpilation #73

merged 47 commits into from
Mar 21, 2023

Conversation

mkuklis
Copy link
Contributor

@mkuklis mkuklis commented Jun 30, 2022

https://issues.folio.org/browse/STRWEB-69

This PR introduces support for transpilation of individual modules. This feature can be accessed via the new transpile command which is now available in stripes-cli (refer to folio-org/stripes-cli#295). Users can now transpile individual modules by running:

yarn stripes transpile.

The transpilation has been set up for all stripes modules:

How to test it

Please copy and run the script below:

mkdir -p transpilation && cd transpilation

# checkout all stripes modules
stripes_modules=(
  components
  connect
  core
  final-form
  form
  smart-components
  util
  webpack
  cli
)

for m in "${stripes_modules[@]}"; do
  git clone "https://github.com/folio-org/stripes-$m.git" --branch transpile &
done

wait

# create workspace via package.json
cat > package.json <<EOF
{
  "private": true,
  "workspaces": [
    "*"
  ],
  "dependencies": {
    "@folio/stripes": "^8.0.0",
    "@folio/users": ">=2.17.0"
  }
}
EOF

# create stripes.config
cat > stripes.config.js <<EOF
module.exports = {
  okapi: {
    url: 'https://folio-snapshot-okapi.dev.folio.org',
    tenant: 'diku',
  },
  config: {
    logCategories: 'core,path,action,xhr',
    logPrefix: '--',
    showPerms: false,
    hasAllPerms: false,
    languages: ['en'],
    suppressIntlErrors: true,
  },
  modules: {
    '@folio/users' : {}
  }
};
EOF

# install dependencies
yarn

modules_to_transpile=(
  "components"
  "smart-components"
  "core"
  "connect"
  "form"
  "final-form"
  "util"
)

for m in "${modules_to_transpile[@]}"; do
  (cd stripes-$m; rm -rf ./dist; yarn transpile) &
done

wait

# start app
yarn stripes serve ./stripes.config.js

@github-actions
Copy link

github-actions bot commented Jun 30, 2022

Jest Unit Test Statistics

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ±0 

Results for commit 1dcc971. ± Comparison against base commit 94a13cf.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jun 30, 2022

BigTest Unit Test Statistics

73 tests  ±0   73 ✔️ ±0   0s ⏱️ ±0s
39 suites ±0     0 💤 ±0 
  1 files   ±0     0 ±0 

Results for commit 1dcc971. ± Comparison against base commit 94a13cf.

♻️ This comment has been updated with latest results.

@mkuklis mkuklis marked this pull request as ready for review March 15, 2023 01:06
@mkuklis mkuklis requested review from zburke and JohnC-80 March 15, 2023 01:07
@mkuklis mkuklis changed the title STRWEB-69 support transpilation STRWEB-69: Add support transpilation Mar 15, 2023
describe('The babel-loader-rule', function () {
describe.only('The babel-loader-rule', function () {
Copy link
Member

Choose a reason for hiding this comment

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

omit .only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zburke thank you for catching this!

@mkuklis mkuklis changed the title STRWEB-69: Add support transpilation STRWEB-69: Add support for transpilation Mar 16, 2023
webpack.config.cli.transpile.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

IIUC, consts.js::defaultExternals needs to be kept in sync with a platform's direct-deps in order to avoid extern'ed things from being duped across UI modules. That feels risky.

Can/should defaultExternals be imported directly from a platform? It bugs me that we have it in two totally separate repos. Maybe having platform-whatever/package.json and platform-whatever/src/consts.js is just as bad. Dunno. Just thinkin'.

'final-form',
'final-form-arrays',
'moment',
'moment-timezone',
Copy link
Member

Choose a reason for hiding this comment

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

moment-timezone isn't currently provided by the platform for apps use as a peer-dep but I think it needs to be for this to work correctly. After this PR, it would be extern'ed in apps but also never pulled in by the platform, and thus absent in the final bundle. (Right?)

'react-dom',
'react-final-form',
'react-final-form-arrays',
'react-final-form-listeners',
Copy link
Member

Choose a reason for hiding this comment

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

Same as moment-timezone; does this need to be added to the platform's direct-deps?

'react-intl',
'react-query',
'react-redux',
'react-router',
Copy link
Member

Choose a reason for hiding this comment

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

Should we add react-router-dom, react-titled, redux-observable, rxjs, zustand?

@mkuklis
Copy link
Contributor Author

mkuklis commented Mar 18, 2023

@zburke the reason we are listing these externals is to make sure they are not included in the transpiled version of any bundle. These are third party dependencies and somehow they need to be included in the bundle by being listed as a dependency of the consumer app (either single ui module or a platform).

I don't think that list belongs to the platform because how would stripes-webpack get access to it? But perhaps this could be extracted to some other place as you suggested.

Some of these libraries (like moment-timezone) are listed as a dependency by the UI modules:

https://github.com/folio-org/ui-users/blob/master/package.json#L911

so that's how they will get fulfilled for now but what happens when all ui modules are also transpiled (with excluded moment-timezone)?

Somehow they will have to be fulfilled by the higher level consumer (in this case the platform).

So I think in short term we should be ok but in long term I think the answer is "yes" the platform will need to list them which I think is actually maybe a good thing? It pushes us to do more cleanup for these 3rd party deps.

Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉 👏 👏 👏

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.

2 participants