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

Add prepublish script and change main #398

Merged
merged 1 commit into from
Dec 13, 2016
Merged

Conversation

budde377
Copy link
Contributor

@budde377 budde377 commented Dec 9, 2016

Building TS before publish and pointing the main entry to the builded files.

Closes #397

@@ -6,7 +6,7 @@
"test",
".gitignore"
],
"main": "dist/xterm.js",
"main": "out/xterm.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you leave this as dist/xterm.js please, since the browserify case is not our main use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course.
But that would entail that you could never use require('xterm') in Node, as we have seen. You would always have to write something like require('xterm/out/xterm.js') which isn't really that pretty. Also, I see no downside with letting main to point to the out/xterm.js. It would still work in the browser using the ´<script src="node_modules/dist/xterm.js"></script>` tag and it would work natively in node..

@@ -32,6 +32,7 @@
"start": "nodemon --watch src --watch addons --watch demo --exec bash -c './bin/build && node demo/app'",
"test": "./bin/build && mocha --recursive ./out",
"build:docs": "jsdoc -c jsdoc.json",
"build": "./bin/build"
"build": "./bin/build",
"prepublish": "./bin/build"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you can use just tsc here? /cc @Tyriar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you are right

@parisk
Copy link
Contributor

parisk commented Dec 9, 2016

Thanks for your contribution @budde377! The proposal is pretty good. Being able to also require the built pre-bundled JavaScript files can definitely have it's use cases.

Now if we decide to ship these files in npm's xterm.js distribution, maybe it's a good idea to change the name of TypeScript's output directory to lib which is a little bit more semantic than out and implies that inside there there are some kind of "library files".

@Tyriar any thoughts on this?

@Tyriar
Copy link
Member

Tyriar commented Dec 9, 2016

That would entail committing lib then as well right? Maybe we should re-consider the release branch? Keeping 2 copies of built files around in the repo feels really messy.

@blink1073
Copy link
Contributor

You can release a sub-folder on npm without committing it, using files: https://docs.npmjs.com/files/package.json#files.

@Tyriar
Copy link
Member

Tyriar commented Dec 9, 2016

@blink1073 didn't know about that

@parisk
Copy link
Contributor

parisk commented Dec 11, 2016

@Tyriar what's your opinion on publishing lib on NPM only, while ignoring this directory in Git.

@Tyriar
Copy link
Member

Tyriar commented Dec 12, 2016

@parisk whatever we can do to keep compiled files out of the master branch is good imo, so if it works 👍

@budde377
Copy link
Contributor Author

Are you guys sure that you want to keep the entrypoint for npm to be dist/xterm.js. See my comment above.

@Tyriar
Copy link
Member

Tyriar commented Dec 12, 2016

If we use files in package.json then I think we should change it to point at lib/xterm.js

@budde377
Copy link
Contributor Author

budde377 commented Dec 12, 2016 via email

@Tyriar
Copy link
Member

Tyriar commented Dec 12, 2016

It's worth bringing up (again) that if we don't need to check in lib for npm support, all that is keeping us checking in dist is Bower which is on the decline. Most devs are moving away from it as npm now downloads deps flatly and Bower is a pain for module owners to support.

@budde377 budde377 force-pushed the feat-npm branch 2 times, most recently from 19940f9 to 12755af Compare December 13, 2016 17:28
@parisk
Copy link
Contributor

parisk commented Dec 13, 2016

@budde377 can you also include the lib directory using the files field in the package.json file?

@Tyriar yep, I think it's time to drop Bower: #406.

Building TS before publish and pointing the main entry to the builded files.
@budde377
Copy link
Contributor Author

budde377 commented Dec 13, 2016

@parisk Done

@parisk
Copy link
Contributor

parisk commented Dec 13, 2016

Works great, thanks!

@parisk parisk merged commit fb17d57 into xtermjs:master Dec 13, 2016
@Tyriar
Copy link
Member

Tyriar commented Dec 13, 2016

Woo! Great result 😄

@blink1073
Copy link
Contributor

Yes! Thank you so much for this.

@logicminds
Copy link

Having the same issue. Despite having this merged I don't understand what I need to do when using browserify to require xterm in my project.

Is this what I need to do in my index.js file?

var xterm = require('./node_modules/xterm/lib/xterm');
var fetch = require('fetch');
var attach = require('./addons/attach/attach.js');
var fit = require('./addons/fit/fit.js');
var fullscreen = require('./addons/fullscreen/fullscreen.js');
var linkify = require('./addons/linkify/linkify.js');

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.

5 participants