-
Notifications
You must be signed in to change notification settings - Fork 36
Fix missing sources in package distribution #97
Conversation
@holgerd77 @s1na Could anyone review this PR? It's a single-line change and it shouldn't take more than a minute or so. |
TBH I had to google this first since I never gave source maps any thoughts on our publishing, this is the first useful reference I found: https://stackoverflow.com/questions/57530478/is-it-recommended-to-publish-source-files-for-typescript-node-modules Seems that we are not approaching this consistently on all our EthereumJS libraries so @rumkin please bear with me and let me give this one more round before merging. @evertonfraga @ryanio @jochem-brouwer @cgewecke what is your stance on this respectively is there some kind of "of course it is done this way" practice in the ecosystem? Should we distribute all packages with the E.g. is there something to consider along using source maps within a |
First. I believe there should be some question framework to make decisions like this. To compare pros and cons faster and in unified way. Because now it seems like no one has a strong opinion on the issue. Second. In my opinion wasting package and disk space with sources is definitely not a good idea. And I would like to provide URLs into source maps to point on some immutable URL, like IPFS, or another package with the sources itself. I'm not sure right now how make it work. But missing sources break builds which require packages which are libraries like RLP. Especially with tools like Webpack, Parcel, etc. This is the bigger harm than wasted disk space IMO. |
Hey @rumkin, I agree source maps are nice to have. I actually just took a look at my dist folder and there are map files generated for me. If this is the case maybe we can just update the package.json to point to it correctly? I am also seeing map files in the vm dist folder. |
@ryanio I meant sources itself. If you open |
@rumkin oh I see you're right, maybe we can configure to emit them at compile time in the tsconfig? |
This PR is fixing missing source with change in |
Could you elaborate a bit on what "breaks builds" means in this context? Background on this question is that I am a bit puzzled that this has not gotten a stronger issue over time. On what type of build setups does this occur? And does "breaking builds" mean that builds are a) not working at all or b) parts of functionality is not working or c) just produce annoying warnings? Thanks! 🙂 |
@rumkin an alternative suggestion (independent from the questions above): the Would it also be a "solution" respectively would this fix the build errors if we remove the |
@holgerd77 Yep, not a correct term for that. Maybe "produce corrupted builds" is better. What I mean is when bundler assemble project especially obfuscated one, it includes dependencies into the final code and realign source maps, to make code inspectable. When there is no sources bundler couldn't provide them with bundle. So developers, when they inspect errors, would not see them in-place in sources panel in DevTools as a typescript, but only as obfuscated JS. Also this is so for regular use via node with Hope it's readable 😅 |
@rumkin ok, thanks for the clarification! 🙂 Would it be a way then - as suggested above - to completely remove the |
Source maps are used for generating proper error trace. While it's not working in Node.js without the flag nowadays and only in browsers, but it would be so in the future. It's reasonable to leave source map for future version of Node.js which will use it. In my opinion it would lead to better debugging and less errors, what should be the first and undeniable priority in crypto development. So my answer is no to that. I would suggest to read and adapt NASA's code development principles for EthereumJS. And I think Practice 34 could be used here as source maps are part of feedback loop in error handling process. While I am a strong opposer to bloated packages, for me the final package size is the lesser evil than hardened debug, especially when people's money depends on it. |
@rumkin Thanks, that's a strong case on this. We'll definitely consider to adopt here (for these kind of questions we unfortunately always need to go into the 24 hours loop due to the distributed nature of the team + different sleep/work hours along 😋 ). |
Yes that definitely works. I haven't tried it yet, but could another option be to set |
@ryanio I have no strong opinion against source inlining. But I think separated files has better UX/DX. You can read source with text editor/IDE from |
@holgerd77 @ryanio Given that we're doing this on the monorepo now, any reason to not add sources for rlp as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me, @holgerd77 if you agree feel free to merge
Yes, I guess rlp is also a strong monorepo-move-over candidate. |
Source map refers to missing source file: '../src/index.ts', what makes source maps useless and lead to errors:
Currently there are
/bin
and/dist
folders bundled. I assume there should be sources too. So I've added/src
dir to package distribution.