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

Fix the condition for isResourceChanged #1301

Merged
merged 1 commit into from
Feb 19, 2023
Merged

Conversation

szegedi
Copy link
Contributor

@szegedi szegedi commented Feb 7, 2023

I was experimenting with this code while building a bit of a showcase for my OSS work and uncovered this bug when loading code from an actual HTTP server and modifying it and testing reloading under modification.

@p-bakker
Copy link
Collaborator

p-bakker commented Feb 8, 2023

On the face it this seems an obvious bug and straight forward fix, so from that perspective I'd say: LGTM

However: could you maybe check the open commonjs issues, to see whether this PR fixes any of them or whether there are additional fixes to be done in this area? https://github.com/mozilla/rhino/issues?q=is%3Aopen+is%3Aissue+label%3ACommonJS

@szegedi
Copy link
Contributor Author

szegedi commented Feb 8, 2023

However: could you maybe check the open commonjs issues

Sure, I can give them a read-over. Here's the results:

#431 is similar but it was already fixed (should be closed, I guess – I can't close it as I had trouble with Mozilla's authentication changes so I lost my maintainer privileges for now.)
#198 commented on it – works as expected, I tried to explain to the user that reloading of source code doesn't affect already loaded exports
#95 WFM, I added a working example in the comments.
#55 is a feature request, supporting node.js packages in addition to CommonJS modules.
#53 and #10 are some Windows issues; #10 seems to be working as expected since the module directory needs to be an URI.

@p-bakker
Copy link
Collaborator

p-bakker commented Feb 8, 2023

Sure, I can give them a read-over. Here's the results:

Tnx a bunch, updated all those issues accordingly

@szegedi
Copy link
Contributor Author

szegedi commented Feb 17, 2023

Tnx a bunch, updated all those issues accordingly

Awesome. Any chance you can merge this one?

@p-bakker
Copy link
Collaborator

Can't, am not a committer :-/

@rbri @gbrail maybe?

@rbri rbri merged commit 7655918 into mozilla:master Feb 19, 2023
@rbri
Copy link
Collaborator

rbri commented Feb 19, 2023

thanks Attila

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.

3 participants