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

THRIFT-5029: Fix Node.js lib entry point #1947

Merged
merged 1 commit into from
Dec 12, 2019

Conversation

antongolub
Copy link
Contributor

@antongolub antongolub commented Nov 24, 2019

The latest published thrift version (0.13.0):

  • does not contain index.js at the package root
  • has no "main" entry point in package.json.

So module request throws an exception:

require('thrift')
Thrown:
Error: Cannot find module 'thrift'
Require stack:
- <repl>
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:625:15)
    at Function.Module._load (internal/modules/cjs/loader.js:527:27)

This PR adds missed "main" field to package.json.

@tedpennings
Copy link

tedpennings commented Dec 12, 2019

➕ for this PR

tedpennings added a commit to lightstep/lightstep-tracer-javascript that referenced this pull request Dec 12, 2019
@dcelasun dcelasun merged commit cf95fef into apache:master Dec 12, 2019
@icebob
Copy link

icebob commented Dec 17, 2019

Same problem.
@antongolub it doesn't fix completely, because of missing also module.exports in thrift.js

@dcelasun
Copy link
Member

PRs welcome.

@sonerokur
Copy link

I think thrift npm package was published from lib/js folder. It should be published from root folder. package.json in root folder references lib/nodejs folder which contains both nodejs and browser bindings.

@bforbis
Copy link
Contributor

bforbis commented Dec 30, 2019

Yeah, I don't believe this change of package.json in lib/js will make a difference, that is not the package.json that is used for nodeJS. Similarly @icebob, since that is browser code it doesn't have module.exports.

@sonerokur
Copy link

I think thrift npm package was published from lib/js folder. It should be published from root folder. package.json in root folder references lib/nodejs folder which contains both nodejs and browser bindings.

Also, i created a JIRA ticket 2 months ago. Any progress on this ? Thanks.
https://issues.apache.org/jira/browse/THRIFT-5039

@jonvuri
Copy link

jonvuri commented Mar 10, 2020

@dcelasun @bforbis or anyone else who has npm maintainer access - can you please fix the state of thrift 0.13.0 as published to npm? It seems to have mistakenly been published using https://github.com/apache/thrift/blob/master/lib/js/package.json when all prior versions were published using https://github.com/apache/thrift/blob/master/package.json, which properly includes and references both the browser and Node.js runtimes in ways that browser and Node.js builds can both understand. I am guessing that this mistaken publishing happened first, prompting this PR, when the real fix would be to republish with the correct package.json at the root.

This has broken all Node.js projects using Thrift for the past several months - for instance, the 'hello.js' example included in the npm Readme. But everything Node trying to require 'thrift' after installing it via npm will get the same error regardless:

{ Error: Cannot find module 'thrift'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:582:15)
    at Function.Module._load (internal/modules/cjs/loader.js:508:25)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:22:18) code: 'MODULE_NOT_FOUND' }

Can you please republish 0.13.0 immediately to correct this? It would be greatly appreciated by those of us trying to stay up to date with Node.js projects, and would make this project work again for any newbies trying to work with the examples.

@dcelasun
Copy link
Member

Unfortunately I don't have npm access. cc @jeking3 @Jens-G

@jonvuri
Copy link

jonvuri commented Apr 7, 2020

@Jens-G @jeking3 Either of you have a spare moment to take a look at this? This can't be a pull request, as it requires an npm maintainer to republish.

@franciscohanna92
Copy link

Need this fix!

@yanfeng42
Copy link

tmp fix diy: fork --> rename --> publish to npm

npm i thrift-tmp --save

then:

rename node_modules/thrift-tmp to node_modules/thrift

@LvChengbin
Copy link

Actually, I think the version 0.13.0 is published with a totally incorrect code base, install 0.12.0 instead.

@emmenlau
Copy link
Member

In order to reduce the pressure on this issue, I've published a snapshot of the current Apache Thrift trunk with many fixes and improvements to https://www.npmjs.com/package/@biodataanalysis/thrift-trunk

Could you please test this to see if it would suite your needs? Any feedback is more than welcome.

@bin-y
Copy link

bin-y commented Jun 15, 2020

Hi @jfarrell @wadey , could you please take a look at this pr and the issue https://issues.apache.org/jira/browse/THRIFT-5039 ?
The package have been broken for about 7 months.

@bin-y
Copy link

bin-y commented Jun 15, 2020

temporary solution:
npm add git://github.com/apache/thrift.git#v0.13.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.