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

Get rid of Immutable.js #3853

Open
smashercosmo opened this issue Jun 3, 2020 · 9 comments
Open

Get rid of Immutable.js #3853

smashercosmo opened this issue Jun 3, 2020 · 9 comments
Labels
area: api area: 3rd party dependencies type: chore work needed to keep the product and development running smoothly
Milestone

Comments

@smashercosmo
Copy link
Contributor

Is your feature request related to a problem? Please describe.

  1. Immutable.js is not maintained
  2. Immutable.js is quite big
  3. Immutable.js makes using netlify-cms less easy, especially when creating page previews, when you have to do things like
function BlogPostPagePreview({
  entry,
  widgetFor,
  fieldsMetaData,
}: {
  entry: any
  widgetFor: any
  fieldsMetaData: any
}) {
  const title = entry.getIn(['data', 'title']) as string
  const author = entry.getIn(['data', 'author']) as string
  const tags = entry.getIn(['data', 'tags']).map((tag: string) => {
    const slug = `/${slugify(tag.toLowerCase())}`
    return { tag, slug }
  })
  const cover = entry.getIn(['data', 'cover']) as string
  const body = widgetFor('body') as React.ReactElement
  const avatar = fieldsMetaData.getIn([
    'author',
    'authors',
    author,
    'avatar',
  ]) as string

  return (
    <BlogPostPage
      title={title}
      body={body}
      author={author}
      avatar={avatar}
      cover={cover}
      tags={tags}
    />
  )
}

Describe the solution you'd like
Standard solution to deal with immutable data structures nowadays is Immer.js
https://github.com/immerjs/immer

Additional context
I know it will be a lot of work, because Immuatble.js is used across the whole codebase. And I really don't know how to introduce it gradually. But I'd really like to help.

@erezrokah
Copy link
Contributor

Huge thumbs up! This is a tech debt, but will require a breaking change so probably for v3

@erezrokah erezrokah added type: chore work needed to keep the product and development running smoothly area: api area: 3rd party dependencies labels Jun 3, 2020
@erezrokah erezrokah added this to the v3 milestone Jun 3, 2020
@erezrokah
Copy link
Contributor

Describe the solution you'd like
Standard solution to deal with immutable data structures nowadays is Immer.js
https://github.com/immerjs/immer

Additional context
I know it will be a lot of work, because Immuatble.js is used across the whole codebase. And I really don't know how to introduce it gradually. But I'd really like to help.

We already have immer as a dependency so we could start by removing non API related usages of immutable.
For example - we could change the reducers, but keep immutable when passing props to widgets.

@smashercosmo smashercosmo changed the title Getting rid of Immutable.js Get rid of Immutable.js Jun 3, 2020
@smashercosmo
Copy link
Contributor Author

Hey, I want to start working on this. Could you guide me a bit? What would be the steps if I, for example, want to remove Immutable from https://github.com/netlify/netlify-cms/blob/master/packages/netlify-cms-core/src/reducers/auth.js?

@erezrokah
Copy link
Contributor

erezrokah commented Jul 9, 2020

Thanks @smashercosmo! I would first search where the auth state is being accessed (probably using grep command on the core package).

For example here is one place:
https://github.com/netlify/netlify-cms/blob/07f47824e960332d9be20588829d3895b32f000b/packages/netlify-cms-core/src/components/App/App.js#L264

Then replace those direct access to the auth immutable instance with selectors.
For example instead of using auth && auth.get('user') have a function that receives the auth state and returns the user:

const selectUser = (auth) => auth && auth.get('user')

export { selectUser } 

We keep selectors in the same file as the relevant reducer, for example:
https://github.com/netlify/netlify-cms/blob/07f47824e960332d9be20588829d3895b32f000b/packages/netlify-cms-core/src/reducers/entries.ts#L636

Once everything is behind selectors, you would mostly need to change the selectors internal implementation and it will be easier to get rid of Immutable.

Does that make sense?

@smashercosmo
Copy link
Contributor Author

Yes. It does. Thank you)

@smashercosmo
Copy link
Contributor Author

@erezrokah ok, I have a PR ready with immutable removed from 'status' and 'auth' state slices. What's the commit type for this kind of change?

@erezrokah
Copy link
Contributor

@smashercosmo that's great. I think you can use refactor: remove immutable.js from status and auth

@mmkal
Copy link
Contributor

mmkal commented Jan 10, 2024

@martinjagodic is this still on the roadmap? Would you accept PRs which refactor immutable.js gradually out? (For all of the reasons stated in this issue, plus 3.5 years of tech debt interest)

@martinjagodic
Copy link
Member

@mmkal that would be amazing, we would be very happy to accept this. However, this is a huge undertaking, so I guess bite-sized PRs would be a way to go. If you want, we can discuss this over Discord in more detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api area: 3rd party dependencies type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

No branches or pull requests

4 participants