Skip to content

Upgrade dependencies #201

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

Merged
merged 2 commits into from
Feb 14, 2023
Merged

Conversation

jtbandes
Copy link
Contributor

@jtbandes jtbandes commented Jan 9, 2023

Fixes #195

This is an attempt at addressing #195. The demo app seems to work, but I had a couple issues I'll need help with:

  • Although the demo app uses React 18, when I tried switching from ReactDOM.render() to the newer createRoot().render() API to fix the deprecation warning, the initial attempt to drag a mosaic window didn't work. However, if I added a new tile before trying to drag, or just tried to drag twice, that did work as expected. Not sure what's going on there, but seems like it might be outside the scope of an upgrade so I left it as ReactDOM.render() for now.
  • I wasn't able to run the unit tests locally:
    $ yarn test:unit
    yarn run v1.22.15
    $ mocha --opts test/mocha.opts 'test/*.ts'
    (node:90942) DeprecationWarning: Configuration via mocha.opts is DEPRECATED and will be removed from a future version of Mocha. Use RC files or package.json instead.
    (Use `node --trace-deprecation ...` to show where the warning was created)
    
    TypeError: Unknown file extension ".ts" for /Users/jacob/Desktop/react-mosaic/test/updatesSpec.ts
        at new NodeError (node:internal/errors:372:5)
        at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:76:11)
        at defaultGetFormat (node:internal/modules/esm/get_format:118:38)
        at defaultLoad (node:internal/modules/esm/load:21:20)
        at ESMLoader.load (node:internal/modules/esm/loader:407:26)
        at ESMLoader.moduleProvider (node:internal/modules/esm/loader:326:22)
        at new ModuleJob (node:internal/modules/esm/module_job:66:26)
        at ESMLoader.#createModuleJob (node:internal/modules/esm/loader:345:17)
        at ESMLoader.getModuleJob (node:internal/modules/esm/loader:304:34)
        at processTicksAndRejections (node:internal/process/task_queues:96:5)
        at async Promise.all (index 0)
        at ESMLoader.import (node:internal/modules/esm/loader:385:24)
        at importModuleDynamicallyWrapper (node:internal/vm/module:437:15)
        at Object.exports.loadFilesAsync (/Users/jacob/Desktop/react-mosaic/node_modules/mocha/lib/esm-utils.js:28:20)
        at singleRun (/Users/jacob/Desktop/react-mosaic/node_modules/mocha/lib/cli/run-helpers.js:149:3)
        at exports.runMocha (/Users/jacob/Desktop/react-mosaic/node_modules/mocha/lib/cli/run-helpers.js:186:5)
        at Object.exports.handler (/Users/jacob/Desktop/react-mosaic/node_modules/mocha/lib/cli/run.js:319:5)
    error Command failed with exit code 1.
    info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
    

Changes proposed in this pull request:

  • Upgrade react-dnd and related packages. Required switching from react-dnd's HOC/decorator APIs to hooks (useDrag, useDrop) and contextTypes -> useContext.
  • Upgrade webpack (because it was choking on a ?? token in a newer version of a dependency)

Reviewers should focus on:

  • A couple minor changes to appease TypeScript, like $set: null -> $set: undefined, and addition of ! in mosaicActions.updateTree(createDragToUpdates(mosaicActions.getRoot()!, ownPath, destinationPath, position));

  • Changes to exported components — some removals, and some changes from class to function components. Although some of these exports seem like they were only for HMR compatibility and not really intended to be part of the public API, this change may necessitate a major version bump to avoid breaking consumers who might've depended on them in some way.

Screenshot

N/A

@jtbandes jtbandes mentioned this pull request Jan 9, 2023
@nomcopter
Copy link
Owner

Thanks for doing this! Will take a look soon. What dependency was webpack choking on? It is likely that package includes sources transpiled to more compatible ES versions that can be mapped to more easily with webpack aliases. Sounds like that went smoothly though so I'll give it a try soon.

@jtbandes
Copy link
Contributor Author

Sorry, forgot which dependency it was exactly. You could probably just revert all the webpack changes and give it a quick try to find out if you're curious – but a webpack upgrade is probably due at some point anyway (although some webpack settings did have to change, so if you'd rather review those separately, maybe you can find some other way to work around the syntax errors with the old webpack version).

@jtbandes
Copy link
Contributor Author

Hey @nomcopter, anything else I can help with here?

@nomcopter
Copy link
Owner

@jtbandes been pushing it forward over the past couple weeks between things - should actually have it ready today pending more webpack nonsense. Really appreciate the work you did and your patience 🙏

@nomcopter nomcopter merged commit 8bcf985 into nomcopter:master Feb 14, 2023
@jtbandes jtbandes deleted the jacob/update-dependencies branch February 14, 2023 00:01
@nomcopter
Copy link
Owner

Man getting these tests working has been quite the journey through ESM hell. I was about to give up.

@jtbandes
Copy link
Contributor Author

Yep, ESM hell might take the # 1 spot for biggest JS PITA even above the React 17->18 upgrade 😅

@nomcopter
Copy link
Owner

Seriously. Released! Let me know how it works for you https://github.com/nomcopter/react-mosaic/releases/tag/v6.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outdated Dependencies
2 participants