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

Vite example #598

Merged
merged 6 commits into from
Mar 30, 2023
Merged

Vite example #598

merged 6 commits into from
Mar 30, 2023

Conversation

curran
Copy link
Contributor

@curran curran commented Mar 2, 2023

Towards #593 by cloning the counter-json1 example and attempting to adapt it to use Vite.

Desired workflows:

  • In dev, npm start to start the server, then in another terminal/tab npm run dev to load the front end with hot reloading enabled (still need to set up the WebSocket proxy for this, but there are blockers in the way of getting to this point)
  • In prod npm run build followed by npm start, like in the other examples.

@curran
Copy link
Contributor Author

curran commented Mar 2, 2023

Current status:

npm run dev should in theory work (aside from the WebSocket connection, which is solvable).

Currently the build results in the following error:

Uncaught TypeError: Cannot read properties of undefined (reading 'prototype')
    at Object.mixin (emitter.js:7:32)
    at node_modules/sharedb/lib/client/doc.js (doc.js:124:9)
    at __require (chunk-RSJERJUL.js?v=7d3727ba:3:50)
    at node_modules/sharedb/lib/client/connection.js (connection.js:1:11)
    at __require (chunk-RSJERJUL.js?v=7d3727ba:3:50)
    at node_modules/sharedb/lib/client/index.js (index.js:1:22)
    at __require (chunk-RSJERJUL.js?v=7d3727ba:3:50)
    at index.js:6:18

Also, a warning is emitted

browser-external:events:9 Module "events" has been externalized for browser compatibility. Cannot access "events.EventEmitter" in client code. See http://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.

This page has some interesting information http://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility, ultimately recommending that client-side libraries expose entry points that don't rely on Node polyfills.

@curran
Copy link
Contributor Author

curran commented Mar 2, 2023

I'm not sure what the best solution for this would be, but ideally ShareDB would provide a build for the client entrypoint that is compatible with Vite and other build systems that do not have built-in support for Node polyfills.

Proposal

Introduce a new asset published to NPM that is a pre-built bundle of the client-side only.

For consumers of ShareDB, instead of saying

import sharedb from 'sharedb/lib/client'

we could say

import sharedb from 'sharedb/lib/client-browser.js'

To produce client-browser.js, the prepublish step of ShareDB itself could include a step that runs sharedb/lib/client through browserify, much like the examples do, which would include the appropriate Node polyfills.

Thoughts?

@curran
Copy link
Contributor Author

curran commented Mar 3, 2023

This is odd.. Node 15 breaks in CI, but the breakage seems unrelated to the new changes:

image

@curran
Copy link
Contributor Author

curran commented Mar 3, 2023

CI breakage is unrelated to this change.

See also:

@coveralls
Copy link

coveralls commented Mar 8, 2023

Coverage Status

Coverage: 97.507% (+0.04%) from 97.471% when pulling c219205 on curran:counter-json1-vite into f4845fd on share:master.

@curran curran marked this pull request as ready for review March 8, 2023 11:27
@curran
Copy link
Contributor Author

curran commented Mar 8, 2023

This is working and ready for review.

Copy link
Contributor

@ericyhwang ericyhwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't used Vite before, but seems straightforward enough! Just one suggestion around the ESLint config.

Re-review should happen without much delay, I was on vacation then got sick, so I'm just now getting caught up.

.eslintrc.js Outdated
Comment on lines 46 to 48
// Support ES6 imports and exports
ecmaVersion: 6,
sourceType: 'module'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this repo in general uses ES3 syntax at the moment, I'd recommend doing a glob override to use this custom config just for the new example, instead of for the entire repo:
https://eslint.org/docs/latest/use/configure/configuration-files#configuration-based-on-glob-patterns

@curran
Copy link
Contributor Author

curran commented Mar 29, 2023

Thanks for the review @ericyhwang ! Feedback addressed.

Copy link
Contributor

@ericyhwang ericyhwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the new example!

@ericyhwang ericyhwang merged commit ab2341e into share:master Mar 30, 2023
@curran
Copy link
Contributor Author

curran commented Mar 30, 2023

Amazing! Thanks a ton.

@curran curran mentioned this pull request Feb 25, 2024
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.

None yet

3 participants