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

Node importing broken #294

Open
bdougherty opened this issue Feb 25, 2023 · 11 comments · May be fixed by #334
Open

Node importing broken #294

bdougherty opened this issue Feb 25, 2023 · 11 comments · May be fixed by #334
Labels
help wanted Extra attention is needed

Comments

@bdougherty
Copy link

I think this was broken by #263.

When importing using import * as Astronomy from 'astronomy-engine'; into a Node project that is marked as a module in package.json or into an .mjs file, it works correctly when using version 2.1.7, but if you use the latest 2.1.15 version, you will get the following error:

/Users/brad/Downloads/test/node_modules/astronomy-engine/esm/astronomy.js:38
export const C_AUDAY = 173.1446326846693;
^^^^^^

SyntaxError: Unexpected token 'export'

It seems to work in both versions when using require.

@matheo
Copy link
Collaborator

matheo commented Feb 25, 2023

@bdougherty I updated the package.json in this way the last time
if you could figure out a valid node_modules/astronomy-engine/package.json for your setup it would be helpful

@cosinekitty
Copy link
Owner

@bdougherty Did the suggestion from @matheo help you? He is the npm expert here, so please let us know if you there is still a problem we can help you with.

@bdougherty
Copy link
Author

Sorry, I haven't had a chance to try anything out yet. I'll let you know what I find out when I'm able to get back to it.

@sebagr
Copy link

sebagr commented Apr 5, 2023

I'm building a project with "type": "module" in package.json, and on top of that I'm browserifying everything (I end up with a 1.2MB build). I often have problems importing stuff probably because of my inexperience with node and browserify, but I ended up doing this:

  1. Copy the esm/astronomy.js file to a different folder and rename it to astronomy.mjs (which lets node know that it's a module)
  2. Import it like this: import * as Astronomy from "./lib/astronomy.mjs";

Everything works as expected, my project builds perfectly, the library works on the browser and all my unit tests are green.

This is far from ideal but hopefully this helps someone move on. I'd love to know when this is fixed so I can go back to relying on the package itself and be able to update it as I would with any other package instead of having to copy the file manually.

@cosinekitty
Copy link
Owner

Is this still broken? I would like to support hassle-free package usage for Node.js, but it's not clear to me how to proceed.

@KenAKAFrosty
Copy link

Had the same issue here in a typescript project. I pulled the source typescript in and used it directly to solve the problem so I'm all good, but I wanted to confirm it is still an issue.

By the way this is an amazing library. The level of clear and apparent care and passion that went into this is astounding! Thank you for the craftsmanship and great work.

@cosinekitty cosinekitty added the help wanted Extra attention is needed label Jul 31, 2023
@cosinekitty
Copy link
Owner

Had the same issue here in a typescript project. I pulled the source typescript in and used it directly to solve the problem so I'm all good, but I wanted to confirm it is still an issue.

As mentioned by @bdougherty creating this issue, apparently 40cd7e3 fixed problems for Deno users but broke stuff for Node users. It looks like the main change is the addition of an "exports" section to package.json, and changing "typings" to "types".

I'm still hoping someone who is an expert on JavaScript module stuff can drop by and help sort this out. This is too far outside my realm of expertise!

By the way this is an amazing library. The level of clear and apparent care and passion that went into this is astounding! Thank you for the craftsmanship and great work.

Hi @KenAKAFrosty and thank you for the kind words.

cosinekitty added a commit that referenced this issue Dec 13, 2023
Include TypeScript type definitions in the exported files.
This might be a fix for #294, but I'm not sure.
The only real way to test is to publish on npm and see what happens!
@talyguryn
Copy link

I have the same problem. Tried to remove export section and it imports fine without any problems.

"exports": {
".": {
"require": "./astronomy.js",
"import": "./esm/astronomy.js",
"types": "./astronomy.d.ts"
}
},

Is it really necessary to define export?

@pep108
Copy link

pep108 commented Feb 8, 2024

@cosinekitty I change the exports to the following, by removing /esm and it works with node. Maybe this minor change is all that's needed.

"exports": { 
   ".": { 
     "require": "./astronomy.js", 
     "import": "./astronomy.js", 
     "types": "./astronomy.d.ts" 
   } 
 },

@cosinekitty
Copy link
Owner

cosinekitty commented Feb 8, 2024

@talyguryn and @pep108, I would like to try either or both of these to close this issue, but unfortunately I do not know enough about the Node/JavaScript world to evaluate whether these changes will work for everyone, or only in the specific cases we are testing.

I'm concerned about continuing to make changes that break existing projects using the astronomy-engine npm package for random subsets of developers who depend on it. You can see this issue started because of my previous attempt to fix usage by Deno in #263. I can visualize this trend continuing of me thinking I have fixed it, only for another issue to be opened later.

I guess what we need is a Node.js expert to come forward and help educate us with best practices for exporting the code and types for a TypeScript-based npm package.

@talyguryn
Copy link

talyguryn commented Feb 8, 2024

As I found to use exports we need to upgrade typescript to 4.7+ and define moduleResolution as node16

Related links:

@talyguryn talyguryn linked a pull request Feb 8, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants