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

fix(table-core): Avoid process not defined in environments not setting a process variable #5373

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

timrittgers
Copy link

When running the library in an environment that is not setting the process variable, the library throws a not defined error and breaks the render. This change checks to see if the variable exists before attempting to read it to avoid this.

@timrittgers timrittgers changed the title Avoid process not defined in environments not setting a process variable fix(table-core): Avoid process not defined in environments not setting a process variable Feb 26, 2024
@KevinVandy
Copy link
Member

What environments do that?

@timrittgers
Copy link
Author

What environments do that?

I have a situation where we are bundling Tanstack into a library component, then distributing the component as a standalone package that can be loaded directly in an html page. In this scenario, there is no process variable defined and the tanstack library fails.

@KevinVandy
Copy link
Member

Are you sure that you're not accidentally importing from src instead of the proper build output?

Also, maybe try importing from the umd output, which does not have any process.env in its output

@timrittgers
Copy link
Author

timrittgers commented Feb 27, 2024

Are you sure that you're not accidentally importing from src instead of the proper build output?

Also, maybe try importing from the umd output, which does not have any process.env in its output

I'm importing the Tanstack library into my project's component like this:

import {
  Cell,
  ColumnDef,
  ColumnSort,
  createColumnHelper,
  getCoreRowModel,
  getFilteredRowModel,
  getPaginationRowModel,
  getSortedRowModel,
  Header,
  PaginationState,
  RowModel,
  Table,
  TableOptions,
  TableState,
} from '@tanstack/table-core';

There wouldn't be a way to just import the pieces I need using the umd, would there?

I'm also curious what the harm would be in detecting the process variable in the library before calling it?

@KevinVandy
Copy link
Member

I'm also curious what the harm would be in detecting the process variable in the library before calling it?

I have just never seen any other NPM library ever have to do this, so I think there should be a better way.

@lschierer
Copy link
Contributor

I hit this trying to use tanstack table in Greenwood

@thescientist13
Copy link

Hey, stumbled across this with @lschierer and did a little digging and have some observations to share if it helps 👋

What environments do that?

browser / client environments, no bundle development workflows, etc.

Also, maybe try importing from the umd output, which does not have any process.env in its output

Tools following NodeJS exports spec looking for ESM files would look to the import conditions, assuming "pure" ESM. In the absence of an exports field, they can optionally look for the module field.

In package.json both the module field and the import sub condition in the exports map field both point to built files that still include process.

(Curious, is there any reason module and import would point to different file outputs? I think they could / should probably be the same for ESM based content)

I have just never seen any other NPM library ever have to do this, so I think there should be a better way.

It's worth noting that the reason most NPM libraries don't guard against this is because most people "erase" it away through their build tools.

For instance, you can see that happening in this project in its examples and own build step


An alternative to guarding would be that for client side / browser / UI libraries don't include any non web standard (e.g. NodeJS APIs)

Not the biggest deal, but hopefully it's helpful information either way. 👍

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.

4 participants