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

Consider to wrap languagePacks.js into self-invoking function #116546

Closed
bpasero opened this issue Feb 12, 2021 · 5 comments
Closed

Consider to wrap languagePacks.js into self-invoking function #116546

bpasero opened this issue Feb 12, 2021 · 5 comments
Assignees
Labels
debt Code quality issues info-needed Issue requires more information from poster

Comments

@bpasero
Copy link
Member

bpasero commented Feb 12, 2021

While working on #116530 I noticed how our 2 modules that are shared in common.js and AMD world are not wrapped in a function scope:

Not wrapping them into a function scope will hoist all functions up when the bundler moves all modules into a single file. They do not end up in the global (window) scope, but there is a chance that we end up with the same function defined multiple times stepping on each other's toes (this is what happened in my PR and was visible in failing integration tests luckily).

I would have just pushed a change to wrap performance.js into a self-invoking function (like I did here: b84ed1f) but then noticed that there seems to be a sharedObj where hoisting seems to be intentional (not sure?). So leaving it up to you to decide if we can simply add a function scope around it or not.

//cc @alexdima

@alexdima alexdima added the debt Code quality issues label Feb 25, 2021
@jrieken
Copy link
Member

jrieken commented Mar 3, 2021

Yeah, that's from a time in which we would support to load performance without any module system into the browser. This isn't needed anymore

jrieken added a commit that referenced this issue Mar 3, 2021
@jrieken jrieken assigned bpasero and unassigned jrieken Mar 3, 2021
@jrieken
Copy link
Member

jrieken commented Mar 3, 2021

Back to you for languagePacks.js

@bpasero bpasero assigned dbaeumer and unassigned bpasero Mar 3, 2021
@bpasero
Copy link
Member Author

bpasero commented Mar 3, 2021

@dbaeumer I think you own that

@bpasero bpasero changed the title Consider to wrap performance.js into self-invoking function Consider to wrap languagePacks.js into self-invoking function Mar 3, 2021
@dbaeumer
Copy link
Member

dbaeumer commented Mar 4, 2021

@bpasero looks like you already did this. See your commit 41339cc

OK to close?

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Mar 4, 2021
@bpasero
Copy link
Member Author

bpasero commented Mar 4, 2021

True

@bpasero bpasero closed this as completed Mar 4, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues info-needed Issue requires more information from poster
Projects
None yet
Development

No branches or pull requests

4 participants