feature: shebang banner #957
Conversation
This comment has been minimized.
This comment has been minimized.
|
not sure why it's failed with node 12 + macOS, I'm currently using those as my local development env and installation + tests are passed locally.. 😂 |
There was a problem hiding this comment.
Sorry but this approach does not work -- per the comments in #338 , the central problem is that the dev/prod entry, index.js was not written as a Rollup plugin and as such is written out-of-band without knowledge of shebangs.
Per the in-line comments below, index.js, which is the bin entry is not changed and the tests do not check for it properly
The correct approach, as I wrote there, is to convert that to a Rollup plugin and do shebang detection as well (no need for a --bin flag, though it could potentially be useful for newbies). Shebang detection was also partially removed at some point in time.
The plugin I'm working on does both of these things
| @@ -41,6 +41,13 @@ describe('tsdx build :: zero-config defaults', () => { | |||
| expect(output.code).toBe(0); | |||
| }); | |||
|
|
|||
| it('should add shebang', async () => { | |||
| const output = execWithCache('node ../dist/index.js build'); | |||
There was a problem hiding this comment.
To test shebangs, you'd want to exec . ./dist/index.js so that the shebang gets used
| it('should add shebang', async () => { | ||
| const output = execWithCache('node ../dist/index.js build'); | ||
| expect(output.code).toBe(0); | ||
| const matched = grep(/#!\/usr\/bin\/env/, ['dist/build-default.*.js']); |
There was a problem hiding this comment.
This test isn't checking the most important file, index.js, which will not have it, which is why this PR will not work
| @@ -5,6 +5,8 @@ interface SharedOpts { | |||
| tsconfig?: string; | |||
| // Is error extraction running? | |||
| extractErrors?: boolean; | |||
| // Is executive? | |||
There was a problem hiding this comment.
| // Is executive? | |
| // Is executable? |
"executive" means something different
| @@ -52,6 +54,7 @@ export interface TsdxOptions extends SharedOpts { | |||
| export interface PackageJson { | |||
| name: string; | |||
| source?: string; | |||
| bin?: string | { [binary: string]: string }; | |||
There was a problem hiding this comment.
should be boolean like the docs and like transpileOnly
|
got it, thanks for clarifying @agilgur5 |
|
@agilgur5 is that ok to use rollup-plugin-preserve-shebang to implement this feature? I can file another PR for it if the idea is acceptable for you |
|
@huozhi unfortunately that will run into the same issue, it won't fix the The optimal solution is a good bit more complicated as it requires a nuanced Rollup plugin and refactoring several internals. I have a branch that does it, but haven't gotten around to testing it or prioritizing it (mostly because of issues written in my current status) |
|
understood, manually generating index.js of cjs bundle is definitely a blocker for that solution. thanks for your patient explanation! 🙏 |
Use
rolloup.output.bannerto add shebang header into bundle. Resolves #338Changes
--binflagbinis specified in package.json, output with shebang header