-
Notifications
You must be signed in to change notification settings - Fork 864
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
Flat bundles #705
base: master
Are you sure you want to change the base?
Flat bundles #705
Conversation
integration/react/package.json
Outdated
@@ -1,7 +1,7 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as flat bundling will be a breaking change (no more reaching out to internal directory structure - which is even proposed in the main READMEs on several occasions) I'd go with simple redux-persist/react
proxy module, I've attempted to keep both redux-persist/integration/react
and redux-persist/react
here but there is really no point in that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, redux-persist/react
is probably best. No issues with breaking changes, although I do have a couple of breaking changes in mind, so we can stack these together for a v6
package.json
Outdated
@@ -2,22 +2,24 @@ | |||
"name": "redux-persist", | |||
"version": "5.6.9", | |||
"description": "persist and rehydrate redux stores", | |||
"main": "lib/index.js", | |||
"module": "es/index.js", | |||
"main": "dist/redux-persist", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is matching first draft of the configs - the ones that are commented out now
package.json
Outdated
"build": "npm run flow-copy && npm run build:commonjs && npm run build:es && npm run build:umd && npm run build:umd:min", | ||
"prebuild": "npm run clean", | ||
"build": "rollup -c", | ||
"build:old": "npm run flow-copy && npm run build:commonjs && npm run build:es && npm run build:umd && npm run build:umd:min", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not entirely sure what's proper way of supporting typings after going flat, "typings" from flow-copy
will stop to work properly, I'd like to explore gen-flow-files
tool (its flow builtin, but ive never used it)
TS will require some work, but at least its a sideway thing (have to be written manually anyway - so it doesnt really matter that much, im concerned about flow because I'd like to generate them automatically and aint sure yet how to do it in a new setup)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure either. gen-flow-files may work, last I tried it was buggy but that was maybe a year ago.
Figuring this out will be a big concern however, having types is important and maintaining separate flow types is not desirable obviously (maintaining .ts declarations is already a big burden).
Is it possible to bundle w/o stripping flow types? then we could basically ship a mirrored redux-persist.flow.js
bundle, which is basically all flow-copy-source does anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately aint sure, need to play with those things first
rollup.config.js
Outdated
exports: 'named', | ||
sourcemap: true, | ||
}, | ||
// const ensureArray = maybeArr => Array.isArray(maybeArr) ? maybeArr : [maybeArr]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part is very much working, although not yet supporting storage
and .native.js
(they can be easily added, I just havent yet got to it)
rollup.config.js
Outdated
umd = false, | ||
env, | ||
} = {}) => ({ | ||
experimentalCodeSplitting: !umd, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this version is interesting though, because it uses code splitting feature - this way babel helpers can be deduped between redux-persist
, redux-persist/storage
and redux-persist/react
flat bundles. I've found a bug in rollup while experimenting with this though - rollup/rollup#1943
It would also require restructuring redux-persist src directory structure slightly, because rollup maps original input filenames to output names (not including directories) and we need stable mapping between those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cant say I understand. So how would src be restructured?
How would babel helpers get deduped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment with this:
input: ['src/index.js', 'src/integration/react.js', 'src/storage/index.js']
It produces 4 files:
- index.js (what will be in main/module)
- index2.js (this should actually be
storage
for readability, that's why we need to restructure a little bit, so input file names get unique names) - react.js
- chunk1.js - here we have magically deduped babel helpers :)
@@ -9,4 +9,8 @@ export { default as getStoredState } from './getStoredState' | |||
export { default as createPersistoid } from './createPersistoid' | |||
export { default as purgeStoredState } from './purgeStoredState' | |||
|
|||
export { default as autoMergeLevel1 } from './stateReconciler/autoMergeLevel1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from what I understand it's what we have established in the other issue, correct me if im wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I think so, but definitely want to get other opinions here. Are there any cases where we cannot rely on code splitting? Will this negatively impact any use cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont think so, I consider this new setup with code-splitting the most "optimized"
@@ -1,5 +1,5 @@ | |||
// @flow | |||
import React, { PureComponent } from 'react' // eslint-disable-line import/no-unresolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was unused
src/storage/index.js
Outdated
@@ -3,3 +3,4 @@ | |||
import createWebStorage from './createWebStorage' | |||
|
|||
export default createWebStorage('local') | |||
export { default as session } from './session' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to add #__PURE__
annotations with my plugin - https://github.com/Andarist/babel-plugin-annotate-pure-calls . So unused version should get dropped. The goal for me here was to provide a single redux-persist/storage
entry
43542b9
to
76fe058
Compare
rollup has released new version with the needed bug fix. Gonna play with this PR further in following days, @rt2zz do you have any comments about what was being done so far (or about what was proposed in the comments)? |
package.json
Outdated
"types": "./src/index.d.ts", | ||
"repository": "rt2zz/redux-persist", | ||
"files": [ | ||
"es", | ||
"src", | ||
"lib" | ||
"lib", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib and es will go away with this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually not, i thought at first they would, but with code splitting on we do not have control over output file names so we need to output files targeting cjs and esm into 2 separate directories to avoid the file path clash
responded. I think the only two considerations I have moving forward are:
|
It depends what it needs to have, i suppose no react & storage in there? Or should storage be available in umd?
That's the only thing I need to experiment with - ain't sure if rollup won't chuck when parsing type nodes.
definitely, this is a breaking change |
76fe058
to
6ed304d
Compare
"integration", | ||
"README.md" | ||
"dist" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
README gets uploaded to npm by default
6ed304d
to
871c61f
Compare
871c61f
to
4edc048
Compare
Ok, I got everything to work. Just one thing is not updated - typescript typings and unfortunately I won't be able to update them right now. Updating them shouldn't take much time though. This is imho ready for final review and ready to be merged. If you have any questions regarding what I've done - just ask :) |
This is not done, you can track progress & discuss what is being done though.