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

Expose entry points exports #453

Merged
merged 2 commits into from
May 1, 2018
Merged

Expose entry points exports #453

merged 2 commits into from
May 1, 2018

Conversation

eXon
Copy link
Contributor

@eXon eXon commented Dec 31, 2017

The way the code is bundled is perfect for the browser. It is also perfect for Node.js if you want to do server-side rendering. However, it was impossible to expose functions that you could import from Node.js by requiring the bundle. The expected behaviour would be to access the entry point exports. Here is an example of what I was trying to accomplish:

test.js

module.exports = function test(req) {
  req.send();
};

index.js

const req = {
  send() {
    console.log('MEOW!');
  }
};

const test = require('./dist/test');
test(req);
  • Step 1: bundling: parcel build test.js
  • Step 2: run node index

The expected behaviour would be to have printed on the console MEOW!. Right now, it would throw test is undefined.

There is many benefits of working this way when you do server-rendering. You still have zero-configuration. The hashed asset names are already converted within the bundle. And many more.

I am not sure if there is only 1 entry point and it is always the first. Maybe someone that knows the code better would be able to confirm if this approach is correct.

@eXon
Copy link
Contributor Author

eXon commented Jan 4, 2018

@davidnagli Does it align with Parcel being just a simple bundler? I've tried hard to make sure there is 0 side-effect on any environment (browser, node.js, electron, etc).

It would make server-rendering much easier. The only missing piece after this would be supporting code-splitting. I'm not sure how this will be possible but we'll see.

@shawwn
Copy link
Contributor

shawwn commented Jan 5, 2018

Hi @eXon,

Interesting PR. I'm trying to run it using these test files: 900e034

Do those files look correct? When I run cd test && ../bin/cli.js build --no-cache integration/node/index.js && node dist/index.js, I get no output.

If you have Slack, hop on and shoot me a message, or join the #contributors channel. https://slack.parceljs.org/

@eXon
Copy link
Contributor Author

eXon commented Jan 7, 2018

Hey @shawwn

I've updated the PR with a more appropriate integration test. The goal is not to bundle the whole thing for Node.js but rather make the exports on the entry point available when you require the bundle from Node.js.

If you run within the node-exports folder npm run build && npm start it should print Test! within the console.

@eXon
Copy link
Contributor Author

eXon commented Jan 7, 2018

@shawwn After testing exactly your code, I can see the MEOW! only if I disable minifying. When minification is enabled, the console.log is stripped away.

screen shot 2018-01-07 at 11 14 43 am

Here is the code with minification enabled:

const e={send(){}},s=require("./test");s(e);

However, in my test the console.log comes from Node.js not the bundle.

@eXon
Copy link
Contributor Author

eXon commented Jan 20, 2018

Anything else I can do to make this PR merged?

Copy link

@krazyjakee krazyjakee left a comment

Choose a reason for hiding this comment

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

The code meets standards. This change doesn't harm existing functionality. It adds functionality to enable SSR capabilities in parcel.

@eXon
Copy link
Contributor Author

eXon commented Feb 18, 2018

@shawwn anything preventing this PR from getting merged? I know everyone is probably busy but I think this would improve parcel and shouldn't cause any problem.

Thanks!


// Expose the exports if we are on the entry point by using the
// previous defined module.
if (previousModule && name === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking name === 1 would break if we ever change the way IDs are generated (such as #780). Not sure how else this can be checked, though. @devongovett ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. Also in split bundles, the entry module does not have id === 1. You can look in the entry argument passed to this function above to get the entry module for the bundle. It is an array, but the last element will be the id of entry module. See here:

if (this.bundle.entryAsset && this.externalModules.size === 0) {
entry.push(this.bundle.entryAsset.id);
}
.

Copy link
Member

@devongovett devongovett 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 your work on this. Sorry for the delay getting this reviewed.


// Expose the exports if we are on the entry point by using the
// previous defined module.
if (previousModule && name === 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah. Also in split bundles, the entry module does not have id === 1. You can look in the entry argument passed to this function above to get the entry module for the bundle. It is an array, but the last element will be the id of entry module. See here:

if (this.bundle.entryAsset && this.externalModules.size === 0) {
entry.push(this.bundle.entryAsset.id);
}
.

req.send('Test!');
}

module.exports = middleware;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding these fixtures! Can you also add a test that uses them in test/javascript.js?

@devongovett
Copy link
Member

Perhaps we should just support generating a UMD bundle instead of something specific for node? That would make it compatible with CommonJS, AMD, globals, etc. https://github.com/umdjs/umd

Here's a template: https://github.com/ForbesLindesay/umd/blob/master/template.js

@codecov-io
Copy link

codecov-io commented Feb 24, 2018

Codecov Report

Merging #453 into master will increase coverage by 1.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #453      +/-   ##
==========================================
+ Coverage   86.61%   87.86%   +1.25%     
==========================================
  Files          77       77              
  Lines        4033     4385     +352     
==========================================
+ Hits         3493     3853     +360     
+ Misses        540      532       -8
Impacted Files Coverage Δ
src/Bundler.js 94.06% <ø> (ø) ⬆️
src/packagers/JSPackager.js 88.57% <100%> (ø) ⬆️
src/assets/StylusAsset.js 71.76% <0%> (-15.2%) ⬇️
src/assets/PugAsset.js 88.57% <0%> (-11.43%) ⬇️
src/assets/SASSAsset.js 90.16% <0%> (-6.62%) ⬇️
src/assets/LESSAsset.js 93.61% <0%> (-6.39%) ⬇️
src/utils/PromiseQueue.js 90.78% <0%> (-3.45%) ⬇️
src/assets/TypeScriptAsset.js 97.29% <0%> (-2.71%) ⬇️
src/utils/serializeObject.js 88.23% <0%> (-1.77%) ⬇️
src/assets/JSONAsset.js 92.3% <0%> (-1.03%) ⬇️
... and 21 more

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 fb6912d...cb837e0. Read the comment docs.

@eXon
Copy link
Contributor Author

eXon commented Feb 24, 2018

@devongovett After reading your comments I've added the missing test and ajusted the entry point to use the correct value. I've also managed to support RequireJS.

While I was reading about UMD examples, I also saw that in order to be UMD, we also need the global export. However, that exporrt needs a name. Just like jquery is on window.jquery, a bundle would be on window.myBundleName. However, as I was trying things out, I was wondering how could we name that?

Maybe this should be only enabled if a library name is specified. Or maybe this can be the package.json file. Should we have this only if we target umd or should we always include this?

I feel like this is an impossible task with the current information the file has. @devongovett Do you have an insight about this?

Thanks!

@eXon
Copy link
Contributor Author

eXon commented Feb 25, 2018

While I was playing with UMD, I think I found the most elegant solution. CommonJS and RequireJS would have the entry point exposed out of the box. Then, the only missing part to be 100% UMD is the browser globals.

By adding a new option --browser-global <variable>, it will allow any author to expose his library by using a global variable and he can choose which one. For example, I'm the author of react-cookie and I want to expose the library on window.reactCookie, I would set --browser-global reactCookie and I would have a 100% UMD build for the browser or Node :).

@devongovett @shawwn @ntomsic @danawoodman
Let me know what you think!

@ghost
Copy link

ghost commented Mar 8, 2018

Any traction on this? This is a much needed feature.

@devongovett devongovett added this to the v1.8.0 milestone Apr 1, 2018
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.

6 participants