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

Create a local bundler to run in development mode #1662

Closed
jgutman opened this issue Oct 23, 2023 · 4 comments
Closed

Create a local bundler to run in development mode #1662

jgutman opened this issue Oct 23, 2023 · 4 comments
Labels
area: data explorer Issues related to Data Explorer category.
Milestone

Comments

@jgutman
Copy link
Contributor

jgutman commented Oct 23, 2023

This issue is motivated by the following:

We are currently blocked on the upgrade to V5 of React Query.
V5 removed the umd directory from the builds, and converted the module type from common JS module to ES module. This isn't a problem for production mode where we use a bundler, but in development mode, we don't have a bundler and so we have some hacks for reading in the local source files created when we build. We can't do this with react query v5, and if any other tanstack libraries do a similar change, we won't be able to upgrade them either.

I think we will ultimately have to solve this by having a bundler run in watch mode for development purposes

Originally posted by @jgutman in #1659 (comment)

This is also seems to be blocking the upgrade to React 18. In v18 of React DOM, some of the critical functionality has moved into react-dom/client, in particular this piece here. To upgrade, we would need to import like this: import * as ReactDOMClient from 'react-dom/client';
However, if we are using our substitute for a bundler and just adding react-dom/umd/react-dom.development.js to the source code, then development mode is not able to find the source code 'react-dom/client' and we get a reference error for ReactDOMClient

This seems like possibly the same issue that prompted this patch in the main application when in development mode:

positron/src/vs/loader.js

Lines 1601 to 1634 in 1f7e74c

// Fixup for loading react-dom/client.js.
if (paths[0].includes('react-dom.production.min.js/client.js')) {
// Save the original path to the react-dom/client.js file.
const reactDomClientOriginalPath = paths[0];
// Release builds load from node_modules.asar.
if (paths[0].includes('/node_modules.asar/')) {
paths[0] = paths[0].replace("/../node_modules.asar/react-dom/umd/react-dom.production.min.js/client.js", "/react-dom/client.js");
} else {
// The set of original paths adjust.
const reactDomClientElectron = '/out/../node_modules/react-dom/umd/react-dom.production.min.js/client.js';
const reactDomClientWeb = 'remote/web/node_modules/react-dom/umd/react-dom.production.min.js/client.js';
// Attempt to adjust the original path.
if (paths[0].endsWith(reactDomClientElectron)) {
paths[0] = `${paths[0].substr(0, paths[0].length - reactDomClientElectron.length)}/out/react-dom/client.js`;
} else if (paths[0].endsWith(reactDomClientWeb)) {
paths[0] = `${paths[0].substr(0, paths[0].length - reactDomClientWeb.length)}out/react-dom/client.js`;
}
}
// Log what happened with loading react-dom/client.js.
console.log('------------------------------------------------------------------------');
if (paths[0] !== reactDomClientOriginalPath) {
console.log(`Changing where the react-dom client.js file is loaded from.`);
console.log(`Original path: ${reactDomClientOriginalPath}`);
console.log(`Adjusted path: ${paths[0]}`);
} else {
console.log('ERROR: Unable to change where the react-dom client.js file is loaded from.')
console.log(`Original path: ${reactDomClientOriginalPath}`);
}
console.log('------------------------------------------------------------------------');
}
// --- End Positron ---

but unfortunately we can't just switch to adding react-dom/client.js from the extension's node_modules folder instead, that breaks as well.

@jgutman
Copy link
Contributor Author

jgutman commented Oct 23, 2023

@jmcphers and @softwarenerd let me know if you have any great insights on this. There's not an immediate need to upgrade, but as time goes on I think these limitations are going to get more painful (the react 18 release has been out already over 1.5 years, and is used by all base Positron components)

@jgutman
Copy link
Contributor Author

jgutman commented Oct 23, 2023

I think this might also prevent me from adding react-query-devtools, even for v4, which is sorely needed to help debug some of the incorrect behavior in our infinite queries. But due to some of the issues/changes discussed here, React DOM is unable to recognize ReactQueryDevtools as a valid React component when we source in the library from build/umd/index.development.js

For example if we do:

import * as ReactQuery from '@tanstack/react-query';
import { ReactQueryDevtools } from '@tanstack/react-query-devtools';
console.log(`Devtools type: ${typeof(ReactQueryDevtools)}`);
console.log(`QueryClientProvider: ${typeof(ReactQuery.QueryClientProvider)}`);

The console should confirm these are both functions that can be parsed as React Components, but the type for the ReactQueryDevtools shows up as a plain Object, and thus the entire library is basically unusable

In general it seems like relying on umd/index.development.js to exist and follow a certain format is very fickle and only works when developers specifically don't meet certain ESM standards

@jgutman
Copy link
Contributor Author

jgutman commented Oct 24, 2023

Locally running npx webpack-cli watch --mode development --config extension.webpack.config.js in both extensions/positron-data-viewer and extensions/positron-data-viewer/ui directories confirms that these libraries (at least react and react-dom/client, which I tested so far) work properly in the data viewer when we have a bundler running in watch mode. I don't know enough though about how to create a gulp task that does the same thing though. Our current watch task is something like npx gulp watch-extension:positron-data-viewer watch-extension:positron-data-viewer-ui where the watch-extension task type is defined by VS code, so I don't know how to do this more automatically (or if we define a new task in scripts and somehow add that

@juliasilge
Copy link
Contributor

juliasilge commented Oct 30, 2023

This change is more about building Positron than any specific user behavior, but I can confirm that as of build 1453 the viewer still works as expected:

viewer

@wesm wesm added the area: data explorer Issues related to Data Explorer category. label Feb 29, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: data explorer Issues related to Data Explorer category.
Projects
None yet
Development

No branches or pull requests

4 participants