Material-ui: [valueLink] React is deprecating the usage

Created on 11 Jan 2016 · 20Comments · Source: mui-org/material-ui

React has merged a pull request deprecating the usage of the valueLink for the input component.
I think that we should follow this for our component.

The first step is to make sure all of our components work in a controlled way without the valueLink property. For instance, this is not the case with this component.

The second step is to add a warning for people using the property.

The last step is to remove the property logic from our component.

The List:

[ ] Checkbox.js
[ ] DatePicker.js (valueLink removed in #3739)
[ ] DropDownMenu.js
[ ] EnhancedTextarea.js (nathan)
[ ] List/MakeSelectable.js (formerly selectable-enhance.jsx)
[ ] Menu.jsx
[ ] Tabs.jsx
[x] TextField.jsx #3699
[ ] Toggle.jsx

umbrella

Source

oliviertassinari

Most helpful comment

I could write 20 scripts to automate it for people in the time we've spent discussing it

Like this? :smile:

import fs from 'fs';
import rrs from 'recursive-readdir-sync';
import "babel-polyfill";

const sourceDir = 'material-ui/src/';
const docsDir = 'material-ui/docs/';
const testsDir = 'material-ui/test/';
const excludeFiles = [
  'MuiThemeProvider.jsx',
  'muiThemeable.jsx',
];

function toPascalCase(string) {
  return string.replace(/(?:^|-)(\w)/g, function (matches, letter) {
    return letter.toUpperCase();
  });
};

function isInArray(array, value) {
  return array.indexOf(value) > -1;
};

function rename(sourceFile, targetFile) {
  fs.renameSync(sourceFile, targetFile, (err) => {
    if (err) throw err;
    console.log('Renamed ' + sourceFile + ' => ' + targetFile);
  });
};

function updateMovedFileRelativePaths(file) {
  const data = fs.readFileSync(file, 'utf-8');
  const newData = data.replace(/(^import .* from \')\.(.*)/gm, '$1..$2');
  if (newData !== data) {
    fs.writeFileSync(file, newData, 'utf-8');
    console.log('Rewrote dependency paths for', file);
  }
};

function updateDependantFileRelativePaths(file, sourceImportName, importName) {
  const regex = new RegExp('(^import\\s' + importName + '\\sfrom.*)' + sourceImportName,'gm');
  const data = fs.readFileSync(file, 'utf-8');
  const newData = data.replace(regex, '$1' + importName);
    if (newData !== data) {
    fs.writeFileSync(file, newData, 'utf-8');
    console.log('Rewrote paths for', file);
  }
};

function updateDependantFileRelativeRawPaths(file, sourceImportName, importName) {
  const regex = new RegExp('(^import\\s.*\\sfrom\\s\'!raw!material-ui\\/lib\\/)' + sourceImportName,'gm');
  const data = fs.readFileSync(file, 'utf-8');
  const newData = data.replace(regex, '$1' + importName + '/' + importName);
    if (data !== newData) {
    fs.writeFileSync(file, newData, 'utf-8');
    console.log('Rewrote raw paths for', file);
  }
};

fs.readdir(sourceDir, (err, items) => {
  if (err) throw err;

  for (let i = 0; i < items.length; i++) {
    // if the file ends with .jsx, and isn't in the exclude list
    if ( (items[i]).endsWith('.jsx') && !isInArray(excludeFiles, items[i]) ) {
      const sourceImportName = items[i].slice(0, -4);
      const targetFileName = toPascalCase(items[i]);
      const targetDirName = targetFileName.slice(0, -4);
      const targetDir = sourceDir + targetDirName + '/';
      const sourceFilePath = sourceDir + items[i];
      const targetFilePath = targetDir + targetFileName;

      // If the targetDir doesn't already eist
      if (!fs.existsSync(targetDir)) {

        // Make the targetDir
        fs.mkdirSync(targetDir);

        // Move the source file
        rename(sourceFilePath, targetFilePath);
        updateMovedFileRelativePaths(targetFilePath)

        // Create a redirect file (or deprecated export)
        const redirectContent =
`import ${targetDirName} from './${targetDirName}';

export default ${targetDirName};
`;
        // Source file redirect
        fs.appendFileSync(sourceFilePath, redirectContent);

        // Target dir index file
        fs.appendFileSync(targetDir + 'index.js', redirectContent);

        // Update component dependant paths
        rrs(sourceDir).forEach((file) => {
          updateDependantFileRelativePaths(file, sourceImportName, targetDirName)
        });

        // Update tests dependant paths
        rrs(testsDir).forEach((file) => {
          updateDependantFileRelativePaths(file, sourceImportName, targetDirName)
        });

        // Update docs dependant paths
        rrs(docsDir).forEach((file) => {
          updateDependantFileRelativePaths(file, sourceImportName, targetDirName)
          updateDependantFileRelativeRawPaths(file, sourceImportName, targetDirName)
        });

        // Otherwise skip file
      } else {
        console.log('Cannot process', items[i], '-', targetDir, 'exists.')
      };
    };
  };
});

mbrookes on 15 Mar 2016

👍2 🎉1

All 20 comments

Seems like these are the components that currently accept valueLink as a prop.

Edit by @alitaheri: Move to issue description.

newoga on 12 Jan 2016

It's now official and into the v15.0.0 pre-release.

LinkedValueMixin and valueLink are now deprecated due to very low popularity. If you need this, you can use a wrapper component that implements the same behavior: react-linked-input.

oliviertassinari on 8 Mar 2016

Yeah, just read the blog. We gotta move fast :runner: :runner: :car:

alitaheri on 8 Mar 2016

@alitaheri Can I help here or do you guys want to take care of this?

nathanmarks on 8 Mar 2016

We should deprecate every usage of valueLink where are are alternative. and provide alternatives where there aren't and the deprecate. it's a bit of work. but we MUST make it to the 0.15.0 release.

All help is appreciated :sweat_smile:

alitaheri on 8 Mar 2016

@alitaheri

Why are we wasting our time deprecating a useless prop in a "major" release? We don't have the manpower of a company like Facebook and I don't know many libs this large that have such a heavy handed deprecation/compatibility policy for breaking releases (nevermind pre 1.0).

The whole point of being pre-1.0 is to say, hey, this isn't ready yet (and it isn't for a lot of use cases -- look at the performance issues), so we need make a bunch of breaking releases while the API reaches a stable place.

The entire thing around here about deprecating till the cows come home is similar to premature optimization. I'm sure that our users would rather we cut the fluff and focus on improving important parts of the library such as performance instead of spending hours discussing the best way to create a clean file setup with backwards compatible mappings -- also a waste of time imo... I could write 20 scripts to automate for it people in the time we've spent discussing it, which isn't even needed for a major release.

I don't even remember how to use valueLink :smile: What do you think, can we just get rid of it for the major release? There's a reason this issue isn't complete yet :stuck_out_tongue_winking_eye:

nathanmarks on 15 Mar 2016

I could write 20 scripts to automate it for people in the time we've spent discussing it

Like this? :smile:

import fs from 'fs';
import rrs from 'recursive-readdir-sync';
import "babel-polyfill";

const sourceDir = 'material-ui/src/';
const docsDir = 'material-ui/docs/';
const testsDir = 'material-ui/test/';
const excludeFiles = [
  'MuiThemeProvider.jsx',
  'muiThemeable.jsx',
];

function toPascalCase(string) {
  return string.replace(/(?:^|-)(\w)/g, function (matches, letter) {
    return letter.toUpperCase();
  });
};

function isInArray(array, value) {
  return array.indexOf(value) > -1;
};

function rename(sourceFile, targetFile) {
  fs.renameSync(sourceFile, targetFile, (err) => {
    if (err) throw err;
    console.log('Renamed ' + sourceFile + ' => ' + targetFile);
  });
};

function updateMovedFileRelativePaths(file) {
  const data = fs.readFileSync(file, 'utf-8');
  const newData = data.replace(/(^import .* from \')\.(.*)/gm, '$1..$2');
  if (newData !== data) {
    fs.writeFileSync(file, newData, 'utf-8');
    console.log('Rewrote dependency paths for', file);
  }
};

function updateDependantFileRelativePaths(file, sourceImportName, importName) {
  const regex = new RegExp('(^import\\s' + importName + '\\sfrom.*)' + sourceImportName,'gm');
  const data = fs.readFileSync(file, 'utf-8');
  const newData = data.replace(regex, '$1' + importName);
    if (newData !== data) {
    fs.writeFileSync(file, newData, 'utf-8');
    console.log('Rewrote paths for', file);
  }
};

function updateDependantFileRelativeRawPaths(file, sourceImportName, importName) {
  const regex = new RegExp('(^import\\s.*\\sfrom\\s\'!raw!material-ui\\/lib\\/)' + sourceImportName,'gm');
  const data = fs.readFileSync(file, 'utf-8');
  const newData = data.replace(regex, '$1' + importName + '/' + importName);
    if (data !== newData) {
    fs.writeFileSync(file, newData, 'utf-8');
    console.log('Rewrote raw paths for', file);
  }
};

fs.readdir(sourceDir, (err, items) => {
  if (err) throw err;

  for (let i = 0; i < items.length; i++) {
    // if the file ends with .jsx, and isn't in the exclude list
    if ( (items[i]).endsWith('.jsx') && !isInArray(excludeFiles, items[i]) ) {
      const sourceImportName = items[i].slice(0, -4);
      const targetFileName = toPascalCase(items[i]);
      const targetDirName = targetFileName.slice(0, -4);
      const targetDir = sourceDir + targetDirName + '/';
      const sourceFilePath = sourceDir + items[i];
      const targetFilePath = targetDir + targetFileName;

      // If the targetDir doesn't already eist
      if (!fs.existsSync(targetDir)) {

        // Make the targetDir
        fs.mkdirSync(targetDir);

        // Move the source file
        rename(sourceFilePath, targetFilePath);
        updateMovedFileRelativePaths(targetFilePath)

        // Create a redirect file (or deprecated export)
        const redirectContent =
`import ${targetDirName} from './${targetDirName}';

export default ${targetDirName};
`;
        // Source file redirect
        fs.appendFileSync(sourceFilePath, redirectContent);

        // Target dir index file
        fs.appendFileSync(targetDir + 'index.js', redirectContent);

        // Update component dependant paths
        rrs(sourceDir).forEach((file) => {
          updateDependantFileRelativePaths(file, sourceImportName, targetDirName)
        });

        // Update tests dependant paths
        rrs(testsDir).forEach((file) => {
          updateDependantFileRelativePaths(file, sourceImportName, targetDirName)
        });

        // Update docs dependant paths
        rrs(docsDir).forEach((file) => {
          updateDependantFileRelativePaths(file, sourceImportName, targetDirName)
          updateDependantFileRelativeRawPaths(file, sourceImportName, targetDirName)
        });

        // Otherwise skip file
      } else {
        console.log('Cannot process', items[i], '-', targetDir, 'exists.')
      };
    };
  };
});

mbrookes on 15 Mar 2016

👍2 🎉1

:smile: :smile:

nathanmarks on 15 Mar 2016

@nathanmarks you make some good points! well about valueLink I absolutely agree. But for some parts of the code it get's hard to migrate, we have to point people to the right place. and for some other we can't even show warnings. But I can't say you're wrong at all. Let's see what others think :+1:

Only one caveat though :grin: Some components work ONLY with valueLink

alitaheri on 15 Mar 2016

@alitaheri I think then as you said it's about assessing things on a case by case basis.

For example just now I was adding the standardized callback to <TextField />. IMO we can strip valueLink right out of there! Can I? pretty please? :beer: :smile:

nathanmarks on 15 Mar 2016

For example just now I was adding the standardized callback to . IMO we can strip valueLink right out of there! Can I? pretty please? :beer: :smile:

Go to town :sunglasses: :bomb: :boom:

alitaheri on 15 Mar 2016

Removing properties without deprecations was what we used to do before React v0.14.
I'm fine with getting rid of valueLink without deprecation but that's rud :bomb:.

oliviertassinari on 17 Mar 2016

@oliviertassinari If we deprecate and still have it passed down, then react will always complain. unless we release 0.16.0 very fast, before the react 15.

I know it's not a good practice to remove props without deprecation. But react somewhat already declared them deprecated a long time ago.

alitaheri on 17 Mar 2016

I've updated the file naming in the list at the top, and added #3739, since my fork has a rewrite of Nitin's rewrite of DatePicker (Yo dawg... :smile:), which includes removing valueLink.

What's the plan for the rest now React 15.0.0 is out?

mbrookes on 8 Apr 2016

What's the plan for the rest now React 15.0.0 is out?

People that use this feature will realize that it's deprecated by React with v15.0.0 and will stop using it.

One option is to deprecate our link with v0.15.x. That should be straightforward. Then remove it with v0.16.0.
The other option is to just to get rid of it with v0.15.0.

I would prefere option 1 so we can ship v0.15.0 earlier.
But I'm pretty sure @nathanmarks is for option 2.
What do you think is best?

oliviertassinari on 9 Apr 2016

I think we're all agreed that we can let React handle the deprecation warning for 0.15.0 release, so there's no urgency to remove valueLink, then we can remove it for a future release.

@newoga did some research, and the only thing we absolutely need to fix for 0.15.0 is List/MakeSelectable.

mbrookes on 9 Apr 2016