Upgrade node to 16.13

[coltoneshaw]

Summary

Started trying to build a plugin and hit a few roadblocks with npm versions and the packages. Below is updating everything to work from the start on the latest node/npm version to hopefully make starting a plugin easier!

• Increased the versions to the latest across all the possible packages. React version remains at v16.14.0 because enzyme does not have React v17 support officially.
• Rebuilt the package-lock.json file with the new versions.
• Updated the docs to include a version number for node / npm.
• Made a few small changes to the webpack config to clean up the isDev conditional.
• Added the babel plugin to eslint to support the latest webpack.
• removed the Mattermost-redux dependency
• Built with Node 16.13 to maintain LTS

[coltoneshaw]

Okay, all tests pass locally. The exception is npm run build which seems to require export NODE_OPTIONS=--openssl-legacy-provider before it to properly run. I think the node version on ci needs to be updated?

coltonshaw@Coltons-MacBook-Pro webapp % export NODE_OPTIONS=--openssl-legacy-provider
coltonshaw@Coltons-MacBook-Pro webapp % npm run build                                

> build
> webpack --mode=production

asset main.js 23.6 KiB [emitted] [minimized] (name: main)
orphan modules 1.04 KiB [orphan] 2 modules
runtime modules 221 bytes 1 module
modules by path ./node_modules/core-js/internals/*.js 54 KiB
  ./node_modules/core-js/internals/export.js 2.54 KiB [built] [code generated]
  ./node_modules/core-js/internals/global.js 590 bytes [built] [code generated]
  ./node_modules/core-js/internals/function-call.js 206 bytes [built] [code generated]
  ./node_modules/core-js/internals/is-object.js 154 bytes [built] [code generated]
  ./node_modules/core-js/internals/is-callable.js 160 bytes [built] [code generated]
  ./node_modules/core-js/internals/get-built-in.js 358 bytes [built] [code generated]
  ./node_modules/core-js/internals/engine-v8-version.js 850 bytes [built] [code generated]
  ./node_modules/core-js/internals/a-callable.js 386 bytes [built] [code generated]
  ./node_modules/core-js/internals/well-known-symbol.js 1.03 KiB [built] [code generated]
  + 88 modules
./src/index.tsx + 2 modules 1.46 KiB [built] [code generated]
./node_modules/core-js/modules/es.promise.js 14 KiB [built] [code generated]
webpack 5.67.0 compiled successfully in 1764 ms
coltonshaw@Coltons-MacBook-Pro webapp % git commit -m "Fixed final jest issue"
[package-update a6e00ec] Fixed final jest issue
 2 files changed, 11 insertions(+), 3 deletions(-)
coltonshaw@Coltons-MacBook-Pro webapp % git push
Enumerating objects: 9, done.
Counting objects: 100% (9/9), done.
Delta compression using up to 12 threads
Compressing objects: 100% (5/5), done.
Writing objects: 100% (5/5), 572 bytes | 572.00 KiB/s, done.
Total 5 (delta 4), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (4/4), completed with 4 local objects.
To https://github.com/coltoneshaw/mattermost-plugin-starter-template.git
   102a40f..a6e00ec  package-update -> package-update
coltonshaw@Coltons-MacBook-Pro webapp % 

Jest

coltonshaw@Coltons-MacBook-Pro webapp % npm run test

> test
> jest --forceExit --detectOpenHandles --verbose

 PASS  src/manifest.test.tsx
  ✓ Plugin manifest, id and version are defined (4 ms)
  ✓ Plugin id and version are defined (2 ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        3.775 s, estimated 4 s
Ran all test suites.
coltonshaw@Coltons-MacBook-Pro webapp % 

Lint

coltonshaw@Coltons-MacBook-Pro webapp % npm run lint

> lint
> eslint --ignore-pattern node_modules --ignore-pattern dist --ext .js --ext .jsx --ext tsx --ext ts . --quiet --cache

coltonshaw@Coltons-MacBook-Pro webapp % 

[hanzei]

CI does indeed use node 14 (https://hub.docker.com/layers/circleci/golang/1.16.0-node/images/sha256-3c826afdfe01aaf82c1bd1a66e723ad7cc1ec1dea67678322eda8751a48cec8a?context=explore). A newer version of the CI image uses node 16.13.1 (https://hub.docker.com/layers/circleci/golang/1.17-node/images/sha256-13cbe95655acf5f0fd4cacbb33211ed70538b041e94f7d370926dd7042b056bd?context=explore) Is that enough?

Other question: Do you know newer node versions can run the existing code? That would be a requirement as all plugins use the same CI version.

[coltoneshaw]

Node 16.13.1 does work for this when i tested the other day. Right now i'm hitting some sha validation issues with react-bootstrap. I'll play with that and maybe just build the package-lock.json for 16.13.

For newer node versions running the older plugin code, it almost entirely depends on the packages installed it seems. Example, the old mattermost-webapp package that was imported in a lot of plugins "github:mattermost/mattermost-webapp#23f5f93d9f12a7e2b5623e5cee6814366abd9a0f" seems to be dated an includes node-sass that isn't compatible with newer node versions. Ideally, those packages are also updated in those plugins.

Side note, I realize I moved the redux link to use the mattermost-webapp package but didn't remove the dependency, let me redo that.

[coltoneshaw]

Okay, rebuild the package-lock.json for node 16.14 and npm 8. Figured that was a better long-term option since node 16 is the LTS version. Everything runs locally just fine. Additionally, removed the mattermost-redux package from the plugin.

I did a test with node 17 and the build fails. If the package-lock is removed, the node packages will install properly.

coltonshaw@Coltons-MacBook-Pro webapp % npm run build

> build
> webpack --mode=production

asset main.js 23.6 KiB [compared for emit] [minimized] (name: main)
orphan modules 1.04 KiB [orphan] 2 modules
runtime modules 221 bytes 1 module
modules by path ./node_modules/core-js/internals/*.js 54 KiB
  ./node_modules/core-js/internals/export.js 2.54 KiB [built] [code generated]
  ./node_modules/core-js/internals/global.js 590 bytes [built] [code generated]
  ./node_modules/core-js/internals/function-call.js 206 bytes [built] [code generated]
  ./node_modules/core-js/internals/is-object.js 154 bytes [built] [code generated]
  ./node_modules/core-js/internals/is-callable.js 160 bytes [built] [code generated]
  ./node_modules/core-js/internals/get-built-in.js 358 bytes [built] [code generated]
  ./node_modules/core-js/internals/engine-v8-version.js 850 bytes [built] [code generated]
  ./node_modules/core-js/internals/a-callable.js 386 bytes [built] [code generated]
  ./node_modules/core-js/internals/well-known-symbol.js 1.03 KiB [built] [code generated]
  + 88 modules
./src/index.tsx + 2 modules 1.46 KiB [built] [code generated]
./node_modules/core-js/modules/es.promise.js 14 KiB [built] [code generated]
webpack 5.67.0 compiled successfully in 1913 ms
coltonshaw@Coltons-MacBook-Pro webapp % npm run lint

> lint
> eslint --ignore-pattern node_modules --ignore-pattern dist --ext .js --ext .jsx --ext tsx --ext ts . --quiet --cache

coltonshaw@Coltons-MacBook-Pro webapp % npm run test

> test
> jest --forceExit --detectOpenHandles --verbose

watchman warning:  Recrawled this watch 17 times, most recently because:
MustScanSubDirs UserDroppedTo resolve, please review the information on
https://facebook.github.io/watchman/docs/troubleshooting.html#recrawl
To clear this warning, run:
`watchman watch-del '/Users/coltonshaw/go/src/github.com/mattermost/mattermost-plugin-starter-template' ; watchman watch-project '/Users/coltonshaw/go/src/github.com/mattermost/mattermost-plugin-starter-template'`

 PASS  src/manifest.test.tsx
  ✓ Plugin manifest, id and version are defined (5 ms)
  ✓ Plugin id and version are defined (6 ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        3.296 s, estimated 4 s
Ran all test suites.
coltonshaw@Coltons-MacBook-Pro webapp %

[coltoneshaw]

What's the motivation for removing the mattermost-redux dependency and switching everything to the web app? I know we generally want to keep some stuff up to date, but by updating it, I'm worrying we're potentially opening ourselves up to more risky and duplicated code in the plugins that is more likely to break going forward. At least by leaving it as-is, we're keeping the same status quo.

The idea here is from @hanzei / @mickmister in order to remove excess packages. Now since it's included in the current web app packages, it seemed like we could just pull from there and keep it up to date. Especially now with the Mattermost-redux repo being archived.

I think this would only really be used for people making new plugins, and the old plugins that exist would have to handle the upgrade how they see fit. Is that what you meant?

[hmhealey]

I guess my core problem here is that I've never thought it was a good idea for plugins to import from the web app since it could affect stability of plugins and potentially of Mattermost itself. Part of the reason for moving mattermost-redux into the web app repo was so that we could provide a cleaner, more stable interface for plugins, but unfortunately since that didn't happen because Web Platform doesn't have the bandwidth, I was hoping we could stick with the old version of mattermost-redux since that was at least stable and unchanging.

[coltoneshaw]

That's fair and I think that shouldn't pose a problem. There aren't any other imports from the web app that are needed at this time (that I can see).

Is the mattermost-redux package updated regularly? That's the only concern I could think of. Since we moved to v6 Mattermost and redux says v5.31, but those could be entirely unrelated versioning.

[coltoneshaw]

Wait, we do import the tests from the webapp

[hanzei]

@hmhealey One reason to import mattermost-redux from webapp is so that plugin can get an up-to-date version. The latest redux version is a year old. Some plugin had to copy code from the webapp just so they can start using newer functionality.

FYI: There are plugins who already import the webapp directly: https://github.com/mattermost/mattermost-plugin-incident-management/blob/8c7f8abc7389b1006eb18c9d2b3d403cb9752bfb/webapp/package.json#L93

[hmhealey]

With it being out of date, I know that there's some new features certain plugins want to use, but I don't think that needs to be part of the starter template since most plugins won't need them. The old version of mattermost-redux includes most common types and things that users would want to do since a lot of the standard stuff around users, posts, and channels haven't changed, so I don't think there's much hard staying with something old and stable. That said, this isn't my area of ownership, so I don't want to block this for that reason any more.

I am 3/5 against removing the lock file though since that will result in dependencies of dependencies changing unexpectedly which could break stuff randomly. I think specifying a fixed commit hash in the package.json and using the method I linked here to skip the buggy integrity check would be a safer way to do that.

[hanzei]

With it being out of date, I know that there's some new features certain plugins want to use, but I don't think that needs to be part of the starter template since most plugins won't need them. The old version of mattermost-redux includes most common types and things that users would want to do since a lot of the standard stuff around users, posts, and channels haven't changed, so I don't think there's much hard staying with something old and stable. That said, this isn't my area of ownership, so I don't want to block this for that reason any more.

I would like to allow for plugin devs picking up the starter-template to not care about the future of there build tools and use the "latest & greatest". This way we also ensure that also advanced plugins can use advanced features.

[hmhealey]

Whoops. I submitted these in the wrong order 😛

That makes sense. I still have my doubts about whether most plugins will need the new stuff, but that's not my call to make anyway. It would be nice if we could have that only affect our code and have it pinned at setup time so that versions don't change down the line, but I guess that would require something more scripted like create-react-app.

Also, sorry, I think I got confused a bit last time. I saw the commit for "Remove root package-lock.json", but I didn't realize that was cleaning up an accidental file. I thought it was removing the actual package-lock.json in the webapp folder

[coltoneshaw]

@hmhealey - Okay, so final resolution for this is to leave it without the redux dependency, and peg the release to an ESR commit for our webapp repo. Is that correct?

@hanzei - I think the CI still needs to have the node version updated to move forward.

[hmhealey]

Yeah, we're continuing with the mattermost-webapp dependency, but I don't remember where we ended up on setting the version. No matter what version we set it to, it'll remain unchanged as long as the person using this template doesn't update it, so I don't have any strong feelings particularly about any specific commit to set it to

[coltoneshaw]

Okay. My thought is that if we leave it open-ended then we run the chance of a plugin dev continuing to update to the latest master branch, without much rhyme or reason. That feels like it could pose an issue.

Wondering if we peg it to the ESR releases, and just update the template here twice a year, and add it to the plugin docs that devs should do the same or follow the plugin channel.

Or is there any chance of making this an NPM package?

@hanzei / @mickmister - You folks are the best advisors on this.

[hmhealey]

Or is there any chance of making this an NPM package?

The end goal will be a few smaller NPM packages that include the shared stuff we need without the massive bloat of mattermost-redux. That was my original plan when we moved it into the web app repo. Unfortunately, we just haven't gotten there yet.

[JulienTant]

Hey guys, is there anything I can do to help get this merged?

[coltoneshaw]

Right now the only thing left is to update the the node version in the tests to have it pass. @hanzei Can you help with that?

I can add a commit to peg this to v6.3 Mattermost web app for right now and maybe a doc on updating it. @mickmister Let me know how you feel about that.

[mickmister]

@coltoneshaw If it compiles, works for me 🙂

Do you mind applying a similar PR to https://github.com/mattermost/mattermost-plugin-demo for testing?

[mickmister]

@hanzei I'd like to get these changes passing with CI, and include the same changes to the e2e tests in the Apps plugin https://github.com/mattermost/mattermost-plugin-apps/pull/323. What changes need to be done to the circleci config in the starter template repo to update the npm version? Is there documentation I can reference to figure this out? I'm not knowledgeable on how our circleci org setup works.

[hmhealey]

FYI, when we next update the web app package, we're going to start running into issues with importing parts of the webapp because of the monorepo. We'll likely going to need to make similar changes to the ones I explain here, although those will be changing a few times over the coming weeks, so perhaps it would be a good idea to wait until those stabilize.

[coltoneshaw]

Made a few more updates here based on removing some dependencies and bringing in the mattermost/@types dep.

Failing on the tests for some EJS issue I can't quite figure out. Only fails when you import the test from the webapp

> test
> jest --forceExit --detectOpenHandles --verbose

 FAIL  src/manifest.test.tsx
  ● Test suite failed to run

    Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/configuration
    For information about custom transformations, see:
    https://jestjs.io/docs/code-transformation

    Details:

    /Users/coltonshaw/Desktop/scripts/work-related/mattermost-plugin-starter-template/webapp/node_modules/@mattermost/webapp/tests/setup.js:4
    import Adapter from 'enzyme-adapter-react-16';
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      2 | // See LICENSE.txt for license information.
      3 |
    > 4 | import '@mattermost/webapp/tests/setup';
        | ^
      5 |

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1728:14)
      at Object.<anonymous> (tests/setup.tsx:4:1)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        1.049 s

[mickmister]

/update-branch

[mickmister]

CI seems to not like when the eslintrc.json file contains ./webapp/tsconfig.json as the path to tsconfig.json, while ./tsconfig.json. Unfortunately this makes the vscode-eslint plugin not work properly.

The latest commit adds a .vscode/settings file, to tell the eslint plugin to use the webapp directory. This is the only way I've gotten it work https://github.com/microsoft/vscode-eslint/issues/1289#issuecomment-873376465

[mickmister]

I temporarily have the webapp setup not being imported in tests, which likely means writing unit tests for React components won't work right now. I couldn't resolve the infamous Cannot use import statement outside a module error this time around. Upgrading jest made it so I could import things, but I ran into other compatibility issues.

I suggest we get this merged, and fix the test setup as a follow-up task. What do you think @coltoneshaw @DHaussermann?

[coltoneshaw]

@mickmister - I'm happy to get this merged whenever you're ready! Let me know what you might need from me

[mickmister]

@hmhealey Can you take a look at some of the changes in the last few commits I've added here? I'm wondering if there's a more proper way to have this set up, the given the use cases of the plugin projects.

[codecov-commenter]

Codecov Report

Base: 5.26% // Head: 5.26% // No change to project coverage :thumbsup:

Coverage data is based on head (dcb3696) compared to base (c0b3c8b). Patch has no changes to coverable lines.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #163 +/- ## ====================================== Coverage 5.26% 5.26% ====================================== Files 3 3 Lines 38 38 ====================================== Hits 2 2 Misses 36 36 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[mickmister]

CI is finally passing :tada: along with VSCode being aware of eslint and typescript

[mickmister]

cc @hanzei Let's get this merged so we have a stable starter template

[hanzei]

@coltoneshaw Please let me know what you think about my non-blocking comments above and then let's merge the PR :rocket:

[mickmister]

@hanzei I think Colton is handing off the PR to our team, so I think any further update should just be done by us, and not wait for his approval or intervention

[coltoneshaw]

@hanzei I think Colton is handing off the PR to our team, so I think any further update should just be done by us, and not wait for his approval or intervention

Feel free to do whatever is needed to have this merged! I have a few other things on my plate and haven't been paying attention to this at the moment. I see the asks are tiny, so if needed I can commit them today so we can move this along.

[hanzei]

@mickmister 1/5 let's merge this PR as it is and clean up the webapp imports later. We can properly also remove the mattermost-redux rules from tsconfig and webpack.

[coltoneshaw]

@hanzei - mattermost-redux alias I added to make it easier to import the code for redux. Without it the imports are kind of ugly / confusing. It's the same thing that Calls does - https://github.com/mattermost/mattermost-plugin-calls/blob/main/webapp/tsconfig.json