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

feat: update build and dev process #1606

Closed
wants to merge 1 commit into from
Closed

feat: update build and dev process #1606

wants to merge 1 commit into from

Conversation

chanceaclark
Copy link
Contributor

Reasons for making this change

Attempt to make the dev and build process more effective. Also export cjs and es modules to enable tree shaking.

If this is related to existing tickets, include links to them as well.
Cleanup from #1539

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

ping @epicfaace

"main": "lib/index.js",
"main": "dist/cjs/index.js",
"module": "dist/es/index.js",
"typings": "dist/src/index.d.ts",
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to add this file in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I was debating on whether to stick a script to emit typings, but I think that'll be good for another PR.

"dist": "npm run build:lib && npm run build:dist",
"lint": "eslint src test playground",
"build": "npm run build:cjs && npm run build:es",
"build:cjs": "NODE_ENV=production BABEL_ENV=cjs babel --extensions \".js,.jsx,.ts,.tsx\" ./src --out-dir ./dist/cjs",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too familiar with cjs vs es. What benefit do we get by building both cjs and es versions of the package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CJS is the old school way of importing/exporting modules. Anyone can just require('@rjsf/core') and it'll work just fine in the browser. Syntax: require(''); module.exports = {}; One of the main benefits for exporting this type of module is for users in which their build processes can't use ES modules or convert their library to use ES modules easily (i.e. gain more usages out of the library).

ESM on the other hand, is the more modern module syntax. It allows for useful features like tree shaking. Currently the library is using ES module syntax. Syntax: import {} from ''; export const foo = '';. Most libraries now-a-days have adopted using ES.

The only big difference from building both is the time it takes to build the library, which I would think it doesn't really matter 🤷‍♀

@@ -12,7 +12,8 @@
"lint": "lerna run lint",
"cs-check": "lerna run cs-check",
"cs-format": "lerna run cs-format",
"dist": "lerna run dist"
"build": "lerna run build",
"start": "lerna run start --stream --parallel"
Copy link
Member

Choose a reason for hiding this comment

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

I made some additional changes in #1607 which allowed one to run npm start from packages/core and then run the playground with the current code, similar to the existing functionality. Would it be possible to update this PR so that that functionality is preserved?

(let me know if there might be a better way of implementing #1607 using the \"npm:build:* --watch\" scripts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may not be understanding what exactly you want, but this root level npm run start might actually be what you're talking about.

If you run npm run start on the root level, it should spin up the playground and live watch the packages/core files and recompile upon save. Is that what you're kind of looking for?

Copy link
Member

Choose a reason for hiding this comment

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

I believe that if you run npm start on the root level, it will watch the packages/core file and run the playground -- but the playground that's running will use the react-jsonschema-form package from npm and not the actual files that are watched/recompiled. Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, let me see if I can get it to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that if you run npm start on the root level, it will watch the packages/core file and run the playground -- but the playground that's running will use the react-jsonschema-form package from npm and not the actual files that are watched/recompiled. Is that right?

unfortunately it does not look like you can run npm run start from the root level (no start defined in package.json)

I can confirm that it uses the playground package from npm when run from the packages/core, as I came across this today, would be good to see if there is a solution to this as I desperately need if then else conditionals as per draft 7 of the json schema spec which OI don't believe is currently implented. I think that having this in will be a more standard compliant way of collapsing of fields as per #268, #304, #598, #920 as basically means if condition is met in data, additional schema is used.

I have no weight here but es6 modules I can understand as they are great for tools such as webpack and rollup allthough not sure if anything is gained building commonjs (browserify???) as I believe the dist files are umd which basically allows commonjs (node), amd (browser i.e requirejs) and straight browser to reference this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have a solution, but I need a better understanding of how you release the docs and playground. @epicfaace would you mind giving me a brief overview?

Copy link
Member

Choose a reason for hiding this comment

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

I've added it in #1639

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for adding the documentation on #1606! Hopefully I can get around to getting this finalized tomorrow.

I know the npm link stuff only works locally, but have we considered using yarn instead? I know with lerna + yarn all that automagical linking will happen under the surface. Here's a quick (outdated) article on it: https://medium.com/@jsilvax/a-workflow-guide-for-lerna-with-yarn-workspaces-60f97481149d.

Copy link
Member

Choose a reason for hiding this comment

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

yarn would definitely be good, though I think we can still stick with npm for now to minimize changes. As per @erunion 's excellent suggestion of using lerna bootstrap (which does this automatic linking either with npm or with yarn), I went ahead and built off your PR to make #1642.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion!

epicfaace added a commit that referenced this pull request Mar 16, 2020
Building off of work done in #1606 and #1641.

Update build and dev process and documentation (as discussed in #1630 (comment)). The developer will just need to run lerna bootstrap, run npm start from the package they are modifying, then run the playground from packages/playground to get a live-reloading playground.
Fix deploy previews by configuring netlify so that it runs lerna bootstrap, thus linking the packages with the local changes in the PR, before building the playground.
Export typings from @rjsf/core (fixes #1583) -- did this in this PR so that @rjsf/material-ui would work as well.
Fix playground issues in which the material-ui playground did not use the right ObjectFieldTemplate or ArrayFieldTemplate.
Fixes #1630 .
@chanceaclark chanceaclark deleted the update-playground branch March 17, 2020 21:45
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.

3 participants