Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 23, 2021

This change does not effect the JS that we output, only the the internal
JS tooling.

One of the interesting changes here is the use of the global object
to share symbols between JS files. In the current setup each JS file
is read in using eval. The only way to share symbol between such
files is via the global namespace which means either a naked variable
declaration (with no const, let or var), or directly using the
global namespace. As a followup we can hopefully remove the use of
eval for the files that are part of this PR.

The long list of exclusions in .eslintrc.yml is because these files
are part of the JS library code which we ship and we a lot of that
cannot be run though the lint tool because it uses our speciall
pre-processing techniques.

This is a followup to #15838 and #15836 which did this for two
individual entry points.

@sbc100 sbc100 force-pushed the more_eslint branch 5 times, most recently from eb6b0d3 to 8b6c398 Compare December 27, 2021 13:25
@sbc100 sbc100 requested review from aheejin and kripken December 28, 2021 10:06
This change does not effect the JS that we output, only the the internal
JS tooling.

This is a followup to #15836 which did this cleanup just for a single
file.

The long list of exclusions in `.eslintrc.yml` is because these files
are part of the JS library code which we ship and we a lot of that
cannot be run though the lint tool because it uses our speciall
pre-processing techniques.
@aheejin
Copy link
Member

aheejin commented Dec 28, 2021

Given that my knowledge of JS is fairly limited, I might ask some basic questions.

One of the interesting changes here is the use of the global object to share symbols between JS files.

Where is this global object defined? Is that our custom object? Can we use globalThis instead?

The only way to share symbol between such files is via the global namespace which means either a naked variable declaration (with no const, let or var),

var variables seem to be global when they are declared outside a function though?

a lot of that cannot be run though the lint tool because it uses our speciall pre-processing techniques.

Do you mean minifying? If so, is there a reason we can't minify linted JS?

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 28, 2021

Given that my knowledge of JS is fairly limited, I might ask some basic questions.

One of the interesting changes here is the use of the global object to share symbols between JS files.

Where is this global object defined? Is that our custom object? Can we use globalThis instead?

global is defined by node itself: https://nodejs.org/api/globals.html#globals_global.

From looking at the docs it looks like globalThis and global are the same thing under nodejs, which means we could use either. I don't think I know enough about node to say which would be better in this particular case... but at least using global will leave the reader knowing for sure that this is a script that is only designed be run under node (we do not support the running of these scripts on the web for example).

The only way to share symbol between such files is via the global namespace which means either a naked variable declaration (with no const, let or var),

var variables seem to be global when they are declared outside a function though?

We may be able to use var instead of assignments to global (or globalThis) but in this change we are moving completely away from the use of var and instead using only the new const and let. They use of var is prohibited by eslint. In addition, I think the use of the global object here make clear the intent to share with other scripts.

a lot of that cannot be run though the lint tool because it uses our speciall pre-processing techniques.

Do you mean minifying? If so, is there a reason we can't minify linted JS?

emscripten's JS library uses a special pre-processor that implements #if and #else and #endif as well as inline {{{ macros }}}. eslint cannot understand this syntax at all sadly.

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

@sbc100 sbc100 merged commit 832e906 into main Dec 29, 2021
@sbc100 sbc100 deleted the more_eslint branch December 29, 2021 10:01
mmarczell-graphisoft pushed a commit to GRAPHISOFT/emscripten that referenced this pull request Jan 5, 2022
…-core#15845)

This change does not effect the JS that we output, only the the internal
JS tooling.

This is a followup to emscripten-core#15836 which did this cleanup just for a single
file.

The long list of exclusions in `.eslintrc.yml` is because these files
are part of the JS library code which we ship and we a lot of that
cannot be run though the lint tool because it uses our speciall
pre-processing techniques.
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