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

Correctly handle exportes when module.exports get's redefined #1015

Merged
merged 2 commits into from
May 7, 2019

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented May 7, 2019

This was broken in 3506ee1, but as this mostly happens in browserified
code I didn't catch it as a problem. Now we get the "exports" once
before we run the script - to be used to resolve import cycles, and once
after the script is ran in order to get the exports if exports gets
redifined in the script.

This is probably broken in the case where both exports is redifined and
there is an import cycle, but the fix for this seems to be some magic
that will need to find out that we are actually requiring a new file and
then to "reexport" the exports before the import? or something like
that.

This was broken in 3506ee1, but as this mostly happens in browserified
code I didn't catch it as a problem. Now we get the "exports" once
before we run the script - to be used to resolve import cycles, and once
after the script is ran in order to get the exports if exports gets
redifined in the script.

This is probably broken in the case where both exports is redifined and
there is an import cycle, but the fix for this seems to be some magic
that will need to find out that we are actually requiring a new file and
then to "reexport" the exports before the import? or something like
that.
@mstoykov mstoykov requested a review from na-- May 7, 2019 13:31
@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #1015 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1015      +/-   ##
==========================================
+ Coverage   72.33%   72.33%   +<.01%     
==========================================
  Files         132      132              
  Lines        9717     9718       +1     
==========================================
+ Hits         7029     7030       +1     
  Misses       2273     2273              
  Partials      415      415
Impacted Files Coverage Δ
js/initcontext.go 97.91% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1621aad...69f5977. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #1015 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1015      +/-   ##
==========================================
+ Coverage   72.33%   72.33%   +<.01%     
==========================================
  Files         132      132              
  Lines        9717     9718       +1     
==========================================
+ Hits         7029     7030       +1     
  Misses       2273     2273              
  Partials      415      415
Impacted Files Coverage Δ
js/initcontext.go 97.91% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1621aad...69f5977. Read the comment docs.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM

@mstoykov mstoykov merged commit 03645ba into master May 7, 2019
@mstoykov mstoykov deleted the fixImportingBrowserifiedJS branch May 7, 2019 14:16
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