status-im / pluto

https://status-im.github.io/pluto/
12 stars 3 forks source link

NodeJS integration branch (progress) #117

Closed tpscrpt closed 5 years ago

tpscrpt commented 5 years ago

@jeluard I apologize for not responding all day, I intended to make some progress at some point today. I've read the message you left on #116 and took it into consideration. Unfortunately, based off the publish.sh script, I can't rebuild effective behaviour for target nodejs.

The most workable outcome I have seen is the current cljsbuild build I've left in project.clj. I recon all of those options should be available in command-line form, like used in publish.sh, I simply can't find appropriate documentation for this issue.

The build works well, at least for coming from a JavaScript background. I'm able to require it as shown in the node.test.js file and then call pluto globally.

Again, I am terribly ignorant of pluto as a project and am inclined to stop working on this unless you can bear with my learning experience.

Thanks!

P.S.: The npm.sh script is outdated, it should reflect the lib folder as output directory and pluto.node.js for the output file.

jeluard commented 5 years ago

Somehow I can see there is some kind of extra npm folder required when using clojure method. Is that what you experience?

The following appears to generate a working file: clj -m cljs.main -t node -d npm/lib -o npm/lib/pluto.node.js -co '{:asset-path "npm/lib"}' -v true -c pluto.js

jeluard commented 5 years ago

BTW all compilers options are there

tpscrpt commented 5 years ago

Alright, completing it within 5 hours.

tpscrpt commented 5 years ago

@jeluard I hadn't seen how to do the -co options you've shown.

tpscrpt commented 5 years ago

My next issue is that the asset path is tricky. I'll try making an index file which requires pluto.node.js as its export or exports those globals specifically.

On Thu, Dec 20, 2018, 4:45 PM jeremiG <notifications@github.com wrote:

@jeluard https://github.com/jeluard I hadn't seen how to do the -co options you've shown.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/status-im/pluto/pull/117#issuecomment-449145607, or mute the thread https://github.com/notifications/unsubscribe-auth/AWfqZV_qaBwvQCZvGK2sBVdNVEK7fw6pks5u7AUSgaJpZM4ZbUFK .

tpscrpt commented 5 years ago

@jeluard Ok, a few notes: the bash script generates npm folder with npm/lib being the dependencies and npm/pluto.node.js being the importable script.

I haven't managed to make pluto.node.js work from outside the project's root. The :asset-path option seems necessary but very annoying to me. Maybe this isn't a requirement for you.

You can test it by modifying index.js with console.log(pluto) and running it with node index.js.

jeluard commented 5 years ago

@JeremiGendron LGTM !

tpscrpt commented 5 years ago

@jeluard I've been messing with the project build script for a few hours and have not yet found the sweet spot.

Here's a thread which explains the problem I'm running into, really the only thing stopping the ability to publish a standard package to npm: https://clojureverse.org/t/target-nodejs-requires-based-off-cwd-instead-of-imported-file/3443

Not sure if you have experience with this, I thought I'd let you know how I've been getting along.

The other approach I've tried is compiling using optimizations, which yields another error but might still be workable. It simply can't resolve react/react-dom and also doesn't expose pluto to the global scope like optimizations :none does.

Just a little update :)

jeluard commented 5 years ago

@JeremiGendron Sorry to see it is so tedious! advanced optimizations is the way to go probably. The export metadata in namespace pluto.js should prevent this name to be dropped. Not sure about the react-dom issue.

tpscrpt commented 5 years ago

alright, checking it out! thanks @jeluard

tpscrpt commented 5 years ago

@jeluard I wasn't able to get it working. I feel like it would take me quite a bit of time to come out with the answer. Would you like me to back down from this task?

jeluard commented 5 years ago

@JeremiGendron Thanks for the effort! I think it's fine for now?

Do you mind just rebasing so that we can merge your PR?

And if you have any ongoing thing or feedback please feel free to share!

tpscrpt commented 5 years ago

Sure thing.

On Sat, Jan 5, 2019, 4:07 AM Julien Eluard <notifications@github.com wrote:

@JeremiGendron https://github.com/JeremiGendron Thanks for the effort! I think it's fine for now?

Do you mind just rebasing so that we can merge your PR?

And if you have any ongoing thing or feedback please feel free to share!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/status-im/pluto/pull/117#issuecomment-451639853, or mute the thread https://github.com/notifications/unsubscribe-auth/AWfqZWNV5OcSQN-u7O7lL03nhJHaYSyyks5vAGtVgaJpZM4ZbUFK .

tpscrpt commented 5 years ago

@jeluard done

jeluard commented 5 years ago

Isn't the package.json needed to publish on npm?

tpscrpt commented 5 years ago

Yes, the name and version fields and optionally the "main" field pointing to the entry script.

On Mon, Jan 7, 2019, 3:19 AM Julien Eluard <notifications@github.com wrote:

Isn't the package.json needed to publish on npm?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/status-im/pluto/pull/117#issuecomment-451855251, or mute the thread https://github.com/notifications/unsubscribe-auth/AWfqZaOGRKWT4C2R1YLx02X-qPR7wGRfks5vAwMugaJpZM4ZbUFK .

jeluard commented 5 years ago

@JeremiGendron Do you ming adding it to this PR? I remember you created one at some point.

tpscrpt commented 5 years ago

Not at all, take note that it still will need to be imported from node_modules/{name}/ and won't work otherwise

jeluard commented 5 years ago

@JeremiGendron Thanks! One last thing, please rebase so that there is a single commit :)

jeluard commented 5 years ago

Thanks!