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

feature: Build ESM alongside CommonJS #413

Closed
wants to merge 1 commit into from
Closed

feature: Build ESM alongside CommonJS #413

wants to merge 1 commit into from

Conversation

pocket7878
Copy link
Contributor

Building EMS module alongside CommonJS to use with webpack + babel.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 28, 2020
@pocket7878 pocket7878 changed the title feature: Build EMS alongside CommonJS feature: Build ESM alongside CommonJS Jun 28, 2020
rollup.config.js Outdated
const config = modes.flatMap(mode =>
moduleFormats.map(moduleFormat => {
let outputConfig;
switch (moduleFormat) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use an array as output, so this switch-case is not necessary.

rollup.config.js Outdated
case 'esm':
outputConfig = {
file: 'esm/recoil.${mode}.js',
format: moduleFormat,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the option is called "es", not "esm" in rollup configs.

rollup.config.js Outdated
switch (moduleFormat) {
case 'esm':
outputConfig = {
file: 'esm/recoil.${mode}.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

How do people use this esm output? import Recoil from 'recoil/esm'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I added "module" property to package.json, many bundle tools (e.g webpack) using module file inside esm directory automatically, so no update import statement is needed.

@pocket7878
Copy link
Contributor Author

@mondaychen
Thank you for your review. I'v updated rollup.config.js !

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mondaychen has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@mondaychen
Copy link
Contributor

The output in esm is expected. I think this PR should be fine:

diff esm/recoil.development.js dist/recoil.development.js
1,2c1,8
< import react from 'react';
< import reactDom from 'react-dom';
---
> 'use strict';
>
> Object.defineProperty(exports, '__esModule', { value: true });
>
> function _interopDefault (ex) { return (ex && (typeof ex === 'object') && 'default' in ex) ? ex['default'] : ex; }
>
> var react = _interopDefault(require('react'));
> var reactDom = _interopDefault(require('react-dom'));
3378,3379c3384,3410
< export default Recoil_index;
< export { Recoil_index_1 as DefaultValue, Recoil_index_2 as RecoilRoot, Recoil_index_3 as atom, Recoil_index_5 as atomFamily, Recoil_index_7 as constSelector, Recoil_index_8 as errorSelector, Recoil_index_26 as isRecoilValue, Recoil_index_22 as noWait, Recoil_index_9 as readOnlySelector, Recoil_index_4 as selector, Recoil_index_6 as selectorFamily, Recoil_index_17 as useGotoRecoilSnapshot, Recoil_index_16 as useRecoilCallback, Recoil_index_18 as useRecoilSnapshot, Recoil_index_12 as useRecoilState, Recoil_index_13 as useRecoilStateLoadable, Recoil_index_19 as useRecoilTransactionObserver_UNSTABLE, Recoil_index_10 as useRecoilValue, Recoil_index_11 as useRecoilValueLoadable, Recoil_index_15 as useResetRecoilState, Recoil_index_14 as useSetRecoilState, Recoil_index_21 as useSetUnvalidatedAtomValues_UNSTABLE, Recoil_index_20 as useTransactionObservation_UNSTABLE, Recoil_index_25 as waitForAll, Recoil_index_24 as waitForAny, Recoil_index_23 as waitForNone };
---
> exports.DefaultValue = Recoil_index_1;
> exports.RecoilRoot = Recoil_index_2;
> exports.atom = Recoil_index_3;
> exports.atomFamily = Recoil_index_5;
> exports.constSelector = Recoil_index_7;
> exports.default = Recoil_index;
> exports.errorSelector = Recoil_index_8;
> exports.isRecoilValue = Recoil_index_26;
> exports.noWait = Recoil_index_22;
> exports.readOnlySelector = Recoil_index_9;
> exports.selector = Recoil_index_4;
> exports.selectorFamily = Recoil_index_6;
> exports.useGotoRecoilSnapshot = Recoil_index_17;
> exports.useRecoilCallback = Recoil_index_16;
> exports.useRecoilSnapshot = Recoil_index_18;
> exports.useRecoilState = Recoil_index_12;
> exports.useRecoilStateLoadable = Recoil_index_13;
> exports.useRecoilTransactionObserver_UNSTABLE = Recoil_index_19;
> exports.useRecoilValue = Recoil_index_10;
> exports.useRecoilValueLoadable = Recoil_index_11;
> exports.useResetRecoilState = Recoil_index_15;
> exports.useSetRecoilState = Recoil_index_14;
> exports.useSetUnvalidatedAtomValues_UNSTABLE = Recoil_index_21;
> exports.useTransactionObservation_UNSTABLE = Recoil_index_20;
> exports.waitForAll = Recoil_index_25;
> exports.waitForAny = Recoil_index_24;
> exports.waitForNone = Recoil_index_23;

@facebook-github-bot
Copy link
Contributor

@pocket7878 has updated the pull request. Re-import the pull request

@pocket7878
Copy link
Contributor Author

I'm just rebased to latest master.

@facebook-github-bot
Copy link
Contributor

@mondaychen merged this pull request in 99b0400.

@mondaychen
Copy link
Contributor

Hmmmm I probably should notice it earlier, but esm/index.js is going to be this file

'use strict';

if (process.env.NODE_ENV === 'production') {
  module.exports = require('./recoil.production.js');
} else {
  module.exports = require('./recoil.development.js');
}

...which is a commonJS file, right? I think we'll need an ESM equivalent for this file.

@mondaychen
Copy link
Contributor

mondaychen commented Jul 4, 2020

I think we'll want to follow how redux does it and provide different builds for CommonJS, ES, ES for browser and UMD dev/prod.

https://github.com/reduxjs/redux/blob/master/rollup.config.js

@pocket7878
Copy link
Contributor Author

pocket7878 commented Jul 4, 2020

@mondaychen

Sorry for my mistake and thank you for your review.
So, should we use something like this (without copying index.js to esm folder):

https://gist.github.com/pocket7878/c7d3bef822e458b2aa71dcb151a56053

@mondaychen
Copy link
Contributor

@pocket7878 can you start a new PR?
If we follow redux's example, we don't need this development and production build for commonJS. We'll not put replace plugin there and leave process.env.NODE_ENV as is. "ES for Browsers" (production), UMD Development and UMD production can use replace.

facebook-github-bot pushed a commit that referenced this pull request Jul 10, 2020
… UMD correctly. (#433)

Summary:
FIX issue from previous PR: #413

Pull Request resolved: #433

Reviewed By: drarmstr

Differential Revision: D22419761

Pulled By: mondaychen

fbshipit-source-id: c9f86304dfc33abd755db249ea9b3d2c30d16ff8
@JeremyRH JeremyRH mentioned this pull request Jul 31, 2020
AlexGuz23 pushed a commit to AlexGuz23/Recoil that referenced this pull request Nov 3, 2022
Summary:
Building EMS module alongside CommonJS to use with webpack + babel.
Pull Request resolved: facebookexperimental/Recoil#413

Reviewed By: drarmstr

Differential Revision: D22348922

Pulled By: mondaychen

fbshipit-source-id: b54a31c76d90b75d49d294ae1583d49a580d329d
AlexGuz23 pushed a commit to AlexGuz23/Recoil that referenced this pull request Nov 3, 2022
… UMD correctly. (#433)

Summary:
FIX issue from previous PR: facebookexperimental/Recoil#413

Pull Request resolved: facebookexperimental/Recoil#433

Reviewed By: drarmstr

Differential Revision: D22419761

Pulled By: mondaychen

fbshipit-source-id: c9f86304dfc33abd755db249ea9b3d2c30d16ff8
snipershooter0701 pushed a commit to snipershooter0701/Recoil that referenced this pull request Mar 5, 2023
Summary:
Building EMS module alongside CommonJS to use with webpack + babel.
Pull Request resolved: facebookexperimental/Recoil#413

Reviewed By: drarmstr

Differential Revision: D22348922

Pulled By: mondaychen

fbshipit-source-id: b54a31c76d90b75d49d294ae1583d49a580d329d
snipershooter0701 pushed a commit to snipershooter0701/Recoil that referenced this pull request Mar 5, 2023
… UMD correctly. (#433)

Summary:
FIX issue from previous PR: facebookexperimental/Recoil#413

Pull Request resolved: facebookexperimental/Recoil#433

Reviewed By: drarmstr

Differential Revision: D22419761

Pulled By: mondaychen

fbshipit-source-id: c9f86304dfc33abd755db249ea9b3d2c30d16ff8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build / infra CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants