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

Stop using all global vars in browser entry #4746

Merged
merged 2 commits into from Sep 17, 2021

Conversation

PaperStrike
Copy link
Contributor

@PaperStrike PaperStrike commented Sep 12, 2021

Description of the Change

module.exports = Object.assign(mocha, global);

is causing chrome to show a 'window.webkitStorageInfo' is deprecated warning.
Using these properties may affect mocha's own functionalities, too. (See #4740)

This PR makes mocha export without using these globals.

Alternate Designs

Why should this be in core?

This bug exists in mocha.

Benefits

No 'window.webkitStorageInfo' is deprecated warning when initializing.
No risk of breaking mocha's functionalities.

Possible Drawbacks

Applicable issues

Fixes #4745.
Fixes #4740.

Is this a breaking change (major release)? no
Is it an enhancement (minor release)? no
Is it a bug fix, or does it not impact production code (patch release)? yes

@coveralls
Copy link

@coveralls coveralls commented Sep 12, 2021

Coverage Status

Coverage remained the same at 94.44% when pulling dda9148 on PaperStrike:limit-global-access into 4860738 on mochajs:master.

@juergba juergba added browser run-browser-test labels Sep 12, 2021
@github-actions github-actions bot removed the run-browser-test label Sep 12, 2021
@juergba
Copy link
Member

@juergba juergba commented Sep 13, 2021

@PaperStrike thanks. I had a look, but havent understood everything, yet. It seems to be quite messy, and I don't want to turn it even worse.

In our Node code (for npm), these functions are static on the Mocha class, and therefore can be acessed by the mocha instance as well. In our browser code, these functions/variables are set on global.Mocha. Why do we need to copy them to global.mocha as well?

Not all these variables have to be set, only a subset depending on which interface (eg. bdd) has been chosen.

@PaperStrike
Copy link
Contributor Author

@PaperStrike PaperStrike commented Sep 13, 2021

Why do we need to copy them to global.mocha as well?

I have no idea. It's not written by me. By the comment, it's for import like in node. However, I can't get this running in real-browsers (bundled with playwright-test), i.e., am not getting global.describe defined (let alone mocha.describe) before a explicit mocha.setup. I searched a long time for where mocha sets these to global before mocha.setup in test cases, tending to make them happen in the mocha instance at the same time, but no luck. Some magic is making them passing the test cases.

That's all I know.

If this is actually broken, delete / skip the required token test case for browser, export mocha without reading these globals, then everyone would be happy.

Or if I understand the situation correctly, we need a brave warrior to do a deep refactor, to better fix this.

@juergba
Copy link
Member

@juergba juergba commented Sep 14, 2021

I searched a long time for where mocha sets these to global [...]

It's in mocha.ui() => this.suite.emit('pre-require', global, null, this);. Note global is the context.
An event listener is registered in lib/interfaces/bdd.js.

@PaperStrike
Copy link
Contributor Author

@PaperStrike PaperStrike commented Sep 14, 2021

@juergba Thanks. I know this and the problem is acturally when mocha invokes the mocha.ui(). In the test cases, global describe, it is always there, set by some point I don't know. But in the browser I tested, this.suite.emit('pre-require', global, null, this); is always invoked latter than browser entry's export. So the last Object.assign of browser-entry.js seems to be there only to make some useless test cases happy.

@juergba
Copy link
Member

@juergba juergba commented Sep 15, 2021

@PaperStrike There is this special test test\unit\required-tokens.spec.js already mentioned in browser-entry.js. Without this test, some other browser tests will fail running on master branch. It's weird, that this test has an influence on other browser tests.

@mantoni can I ask you a question, please?
Do you know why we need this module.exports = Object.assign(mocha, global); Is it for making some useless Mocha tests happy (maybe some debris from the past)? Or something related with browserify? Or for people using Mocha's bundle for bundling their own file? Or whatever?
Why would we need: thus, you can now do "const describe = require('mocha').describe" in a browser context (assuming browserification). ?

@mantoni
Copy link
Contributor

@mantoni mantoni commented Sep 15, 2021

@juergba Mocha cannot be browserified anymore since v8. I don't know why this exists, but if it explicitly mentions browserify, I guess it's legacy.

@PaperStrike
Copy link
Contributor Author

@PaperStrike PaperStrike commented Sep 16, 2021

I made a aggresive commit that removes the required-tokens test case and put browser-entry.js into the setup (test/browser-specific/setup.js, not sure if it is the right place) for other tests that requires it.


And I came across a way that may fix the problem without editing tests by changing the last few lines of browser-entry.js to

Forget about it. My fault.

@juergba juergba added the run-browser-test label Sep 16, 2021
@github-actions github-actions bot removed the run-browser-test label Sep 16, 2021
@juergba
Copy link
Member

@juergba juergba commented Sep 16, 2021

@mantoni thank you

@PaperStrike
Copy link
Contributor Author

@PaperStrike PaperStrike commented Sep 16, 2021

It will be better to have describe, it available on mocha in browsers, anyway. We all agree it is broken currently as we copy from the global before a mocha.setup.

And we can (just tested) achieve this by copying the values from Mocha in the last few lines of browser-entry.js like:

Object.keys(Mocha)
  .filter(function(key) {
    return !(key in mocha);
  })
  .forEach(function(key) {
    mocha[key] = Mocha[key];
  });

module.exports = mocha;

The problem is the test cases (tests in this repo, I mean). It's hard to pass them. I tried to put a require('../..').setup('bdd') in test/browser-specific/setup.js, and found I would need to put a .run somewhere, too. And I don't know where to put it. (Unless setting up as 'tdd', which magically passes the tests. But our tests use bdd, right?)

@juergba
Copy link
Member

@juergba juergba commented Sep 16, 2021

@PaperStrike thank you so far, you are a big help on this issue. I feel a little better, but still this bundling story is so fuzzy.

Why do we need describe and it in mocha at all:

  • for users?: We all agree it is broken currently as we copy from the global before a mocha.setup.
    So you are saying for the user these functions are currently not available on mocha.
    And nobody is complaining. Right?
  • for browserify?: no => Mocha cannot be browserified anymore since v8.
  • for our own tests?: seems to be the only reason.

So at first sight I like the idea to removing the required-tokens requirement:

  • I haven't understood the role of required-tokens test case. Why does it work with, but fails without it?
  • In our Node code (for npm), these functions are static on the Mocha class, and therefore can be acessed by the mocha instance as well. I was wrong, in Node these functions are not available on mocha instance either.
  • The problem is the test cases (tests in this repo, I mean).: why? all CI tests have been passing ...
  • it will be better to have describe, it available on mocha in browsers: why? The users currently don't have it on mocha.

I made a aggresive commit that removes the required-tokens test case and put browser-entry.js into the setup (test/browser-specific/setup.js, not sure if it is the right place) for other tests that requires it.

I haven't understood, why browser-entry.js in the setup makes the test pass.

Is your last proposition as an addition to the current PR? Or standalone?

@PaperStrike
Copy link
Contributor Author

@PaperStrike PaperStrike commented Sep 16, 2021

@juergba

  • I haven't understood the role of required-tokens test case. Why does it work with, but fails without it?

To find the cause, I shortened required-tokens.spec.js as tiny as possible, and found the necessary part, require('../..'). ../.. links to the index.js in the root dir, and by the browser field in package.json, it links to browser-entry.js when bundling. So the solution is require browser-entry.js somewhere. But wait, what's the root cause, and what if we only need a little piece of browser-entry.js? Combined with the error messages (something related to properties of process), and the content of browser-entry.js, it seems better to require browser-entry.js as a whole.

  • In our Node code (for npm), these functions are static on the Mocha class, and therefore can be acessed by the mocha instance as well. I was wrong, in Node these functions are not available on mocha instance either.

Yeah. Earlier this day I found mocha in Node exports Mocha, and mocha in browser exports its instance. Quite weird I would say.

  • The problem is the test cases (tests in this repo, I mean).: why? all CI tests have been passing ...

"The problem" means we will have tests failed, if we use the code right above that sentence, without doing other stuff like removing required-tokens.spec.js and editing test/browser-specific/setup.js. The later sentenses in that comment start with this context, too.

Sorry I didn't made it clear.

  • it will be better to have describe, it available on mocha in browsers: why? The users currently don't have it on mocha.

Modules (without side effects) are the future. I like writting tests like:

import { describe, it } from 'mocha';
describe(...);

which is not possible in browsers. And I wish one day mocha exports functions to global only when the user requires (with an option, maybe). It's not possible for now, and the first step to modularize would be the users being able to write like using modules.

Or if I understand the situation correctly, we need a brave warrior to do a deep refactor, to better fix this.

The "deep refactor" may be to export Mocha in both Node and browsers. Is anyone interested?

@juergba
Copy link
Member

@juergba juergba commented Sep 16, 2021

Ok, we are quite close now.
I prefer the solution of this PR as is, so without describe/it on mocha.

import { describe, it } from 'mocha'; [...] which is not possible in browsers.

Why do you prefer having it on mocha then? If you can't use it anyway. Or can you use it for your own bundling?

Edit: re deep refactor: no, I don't know anybody. I hate browsers.

@PaperStrike
Copy link
Contributor Author

@PaperStrike PaperStrike commented Sep 16, 2021

Why do you prefer having it on mocha then? If you can't use it anyway. Or can you use it for your own bundling?

I can use it, if the code in #4746 (comment) were applied. If it's applied I can write tests like:

<script src="link/to/mocha/bundle"></script>
<script>
mocha.setup('bdd');
window.addEventListener('load', () => {
  mocha.run();
});
</script>
<!-- Can also link to external script files -->
<script type="module" id="my-real-tests">
import { describe, it } from 'link/to/mocha/bundle';

describe('group', () => {
  it('pass', () => {});
});
</script>

Sorry again for my expressions. The "not possible in browsers" I mean it's not possible to use mocha in this way in browsers as of now.

@PaperStrike
Copy link
Contributor Author

@PaperStrike PaperStrike commented Sep 16, 2021

Anyway, it seems a bit off topic now. If you prefer to delete the required-tokens test case and agree to put a require in /test/browser-specific/setup.js, merge this PR then.

@juergba
Copy link
Member

@juergba juergba commented Sep 16, 2021

Ok then, add #4746 (comment) to this PR.
Please also confirm wether #4740 will be fixed, and add it to your description.
I will do some testing tomorrow with your updated branch.

@juergba
Copy link
Member

@juergba juergba commented Sep 16, 2021

The required-tokens test case will not pass, even if you add your proposition.

@PaperStrike
Copy link
Contributor Author

@PaperStrike PaperStrike commented Sep 16, 2021

The required-tokens test case will not pass, even if you add your proposition.

Yeah, that's why I mentioned "The problem is the test cases", "tried to put a require('../..').setup('bdd') in test/browser-specific/setup.js", found 0 test is running for "need to put .run() somewhere, too", and noticed "setting up as setup('tdd') magically passes the tests" while "we're using bdd".

I'm in UTC+8 and sorry I am out for some hours.

@PaperStrike
Copy link
Contributor Author

@PaperStrike PaperStrike commented Sep 17, 2021

@juergba After running some own tests in real browsers without bundling beforehead, now I agreed to keep this PR as is.. Been using these TypeScript, bundlers for sometime, damn me forget the browsers don't support such syntax at all. I can get it work, but I need bundlers. Users using bundlers to run tests are the minority, I believe. Sorry for wasting your time.

@PaperStrike
Copy link
Contributor Author

@PaperStrike PaperStrike commented Sep 17, 2021

PR description updated.

@juergba
Copy link
Member

@juergba juergba commented Sep 17, 2021

Regarding removed test case required-tokens.spec.js: we still have integration tests covering this require interface on Node.

Copy link
Member

@juergba juergba left a comment

@PaperStrike thank you for this PR!

@juergba juergba merged commit 8421974 into mochajs:master Sep 17, 2021
18 checks passed
@juergba juergba added confirmed-bug semver-patch labels Sep 17, 2021
@juergba juergba added this to the next milestone Sep 17, 2021
@PaperStrike PaperStrike deleted the limit-global-access branch Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser confirmed-bug semver-patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants