ianstormtaylor / superstruct

A simple and composable way to validate data in JavaScript (and TypeScript).
https://docs.superstructjs.org
MIT License
7.01k stars 224 forks source link

Breaking change was not declared as such #1139

Open maxadv opened 1 year ago

maxadv commented 1 year ago

This change https://github.com/ianstormtaylor/superstruct/commit/774917717741a38b2489713f1b7a2d3fead3bdca was a breaking change for our project.

We needed to add this webpack rule so that it transpiles to our desired ES8 version. We made the rule more general now, so that the file ending doesn't matter anymore.

        {
          // transpile dependency: superstruct
          test: /node_modules\/superstruct/,
          use: [{ loader: "babel-loader" }],
        },

What would you suggest? What about adding this to some documentation or do you think that this problem is too specific for us?

ianstormtaylor commented 1 year ago

@maxadv hey, sorry for the trouble. Can you explain more about what about that change was breaking or what you were trying to solve by editing your webpack config?

maxadv commented 1 year ago

No worries, just wanted to flag it here in order to maybe find an action item to prevent that in the future. You're right a bit more context would be helpful.

Our clients require that we're fully compatible with ES8. That was our initial webpack configuration to transpile superstruct:

{
          test: /\.(ts|js)$/, // as you can see the new file ending `*.mjs` wasn't supported
          use: [{ loader: "babel-loader" }, { loader: "ts-loader" }],
          // transpile dependencies: superstruct
          exclude: /node_modules\/(?!(superstruct)\/)/,
},

If we don't do it, we get this error by es-check (es-check es2017 './dist/**/*.js' --verbose --module):

ES-Check: there were 2 ES version matching errors
SyntaxError: Unexpected token

which refers to the rest syntax within this class:

class StructError extends TypeError {
  constructor(failure, failures) {
    let cached;
    const {
      message,
      ...rest // this is the problematic line
    } = failure;
   //...
  }
}

We use this babel configuration to solve this issue:

  "babel": {
    "presets": [
      [
        "@babel/preset-env"
      ]
    ],
    "plugins": [
      "@babel/plugin-proposal-object-rest-spread"
    ]
  },

The new webpack configuration solved the issue:

        {
          test: /\.(ts|js)$/,
          use: [{ loader: "babel-loader" }, { loader: "ts-loader" }],
          exclude: /node_modules/,
        },
        {
          // transpile dependency: superstruct
          test: /node_modules\/superstruct\//,
          use: [{ loader: "babel-loader" }, { loader: "ts-loader" }],
        },

I'm not sure how others are using this library or if they just support higher ES versions which wouldn't create this problem.

What about if you export *.js files only to reduce issues also for others, because the type module is already set? https://github.com/ianstormtaylor/superstruct/blob/6717743ee286b13dde6de072019194afa75bfb29/package.json#L3

ianstormtaylor commented 1 year ago

@maxadv thanks, I'll look into using .js since we already have type: 'module'. It looks like tsup would do that out of the box, which seems like a nice simplification of the build process. But it would also use .cjs in that case for the CommonJS file.

I think the sad truth is that for all our file checking we have to include .cjs and .mjs as valid options from now on—so going forward the regex should really be:

/\.(ts|js|cjs|mjs)$/