-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix broken NPM bundle by publishing in ES-Module format #1958
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
Fix broken NPM bundle by publishing in ES-Module format #1958
Conversation
Please don't open a new PR as that means it is more difficult to see all the context. You can just adjust the existing one with your changes. |
@CendioOssman sadly github does not allow changing the head branch of the PR. Your request is to history-rewrite my original PRs head (master)? I can do that but for me it makes it more difficult to understand the context in the future. |
Continue here then, strange behavior by GitHub.. |
package.json
Outdated
"exports": { | ||
".": "./core/rfb.js", | ||
"./lib/*": "./core/*.js", | ||
"./lib/*.js": "./core/*.js", | ||
"./core/*": "./core/*.js", | ||
"./core/*.js": "./core/*.js", | ||
"./lib/vendor/*": "./vendor/*.js", | ||
"./lib/vendor/*.js": "./vendor/*.js", | ||
"./vendor/*": "./vendor/*.js", | ||
"./vendor/*.js": "./vendor/*.js", | ||
"./package.json": "./package.json" | ||
}, |
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.
RFB is our only interface. Do we need anything more than the first line?
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.
There are uses I could find online using the github search:
And since I do not want these inputs to break I have written it as it is there.
I could probably remove the part about vendor, since it does not seem to be used.
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.
Those are all unsupported usages of noVNC. I'm not keen on giving any illusion of that being official and stable.
Ideally, we just have:
"exports": "./core/rfb.js",
Which is the only supported interface at the moment.
The question is what that transition means for people. Given that we are switching from CommonJS to modules, I assume there is some transition work needed anyway? So might be a good idea to do this at the same time?
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.
util/logging.js
is clearly also exposed api, otherwise, the provided example vnc.html would not work:
Line 53 in 4cb5aa4
import * as Log from './core/util/logging.js'; |
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.
As are the files under input/key*
:
Lines 15 to 17 in 4cb5aa4
import KeyTable from "../core/input/keysym.js"; | |
import keysyms from "../core/input/keysymdef.js"; | |
import Keyboard from "../core/input/keyboard.js"; |
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.
Exposed is not the same as supported or stable. I.e. you use them at your own risk. We will not consider it a bug if we change them in a way that breaks callers.
For our internal use, that is fine as we can change it at the same time. But external users cannot really do the same safely.
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.
I'm aware this will cause some pain for some users. But that might be the best thing in the long run, as that will force a discussion of what is missing from the official API and needs to be added.
"directories": { | ||
"lib": "lib", | ||
"doc": "docs", | ||
"test": "tests" | ||
}, |
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.
Are these not used anymore?
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.
See npm js docs: directories
The CommonJS Packages spec details a few ways that you can indicate the structure of your package using a directories object. If you look at npm's package.json, you'll see that it has directories for doc, lib, and man.
In the future, this information may be used in other creative ways.
Since this is no longer exported using ComonJS Modules, I am not sure who would use this.
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 fair. But let's have a separate commit that explicitly states that these are being removed because they are old and unused.
Can probably put the removal of "browser" in the same commit.
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.
Hi 👋
Just checking in to see if there are any updates on this thread
Hey! Just wondering if there’s any ETA on when this might get merged. :) |
damn nobody use this huh |
@elikoga, is this still on your todo list? |
This is still an issue for me. I'm currently having to lock at version 1.5.0 so that I can properly bundle the code. |
I think that @elikoga is doing his best to implement a change, you can test his changes from his repo (https://github.com/elikoga/noVNC/tree/switch-to-es-module-publish-and-use-exports) and let here know if that's suitable for you From a number of projects referencing this broken version (1.6.0 via code builders) - you're not the only one waiting for a fix @CendioOssman Do you need any help in a validation of a changes here? |
Please do. We don't use the npm version ourselves, so feedback from actual users is very welcome. Including on the proposed adjustments in my reviews. |
Ok - so I've created a simple project that uses ViteJS and ReactJS (samples in this gist) - and confirmed an issue with 1.6.0 version So - for web-based apps this change will give a possibility to bundle a code using modern package manages. What other tests shall I perform? |
这个事项还在推进中吗?是否有一个可供期望的发布时间?目前我也是因为这个问题导致不得不停留在v1.5.0 |
I am still available to look at it if needed. I do not know when it will be released. Upstream is not directly concerned with keeping npm up to date due to their releasing mechanisms not going through there. 1.5.0 is still completely fine, unless you need one of the newer features. |
好的,我可以继续使用1.5.0,但还是非常期待这个合并能帮助我升级到1.6.0以上。感谢。 |
@CendioOssman What shall we do to merge that mr? It's been few months to merge so simple (and organisational) change? |
a940f34
to
fe29dc6
Compare
I made the last discussed adjustments and merged this. It will momentarily be available from NPM under the "dev" tag. Please have a look and see how it works. |
Is there another semver release planned soon? |
That's the idea. Hopefully a beta soon. |
I've tested 1.6.0-g44b7489 - lgtm 👍 |
Closes #1943
Repeat of #1944 with different head branch
Testable by placing
"@novnc/novnc": "npm:@elikoga/[email protected]",
inpackage.json
Uses
exports
inpackage.json
to make sure that old imports still workThis one incorporates the review requests in the previous PR by simply dropping the conversion step using babel.
CI Passes in my repo