Skip to content

Changing the wording around module.exports #10692

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

Closed
wants to merge 2 commits into from
Closed

Changing the wording around module.exports #10692

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 22, 2018

Hello!

I just want to point something out here:

Using the commonjs format (require() and module.exports, though node does add some additional things on top of that spec) is not the same, or even roughly the same, which is implied/stated by this line here:

Instead of using require and module.exports like shown above, JavaScript has an alternate syntax, which is a more accepted standard. Choose whichever you want: they do the same thing.

I propose a minor edit here, which is that we actually want to clarify that the ECMAScript standard that has been finalized (often this is known as ES6 or ES2015. There was a minor revision in there but thats long since passed in either case) and one of the purposes, per the Module spec, is that this is the standard for which modules will be native to the browser. (In the evergreen versions of Safari, Chrome, Firefox, and I think even Edge now, you can use <script type="module" src="somemodule.js"> and it will work fine). They are also the only official way of using dynamic imports, as highlighted in your own documentation in a way that I think gets the point across very well.

I think this is very misleading otherwise. You do in fact see a performance decoupling bundling the commonjs modules, as webpack can't take advantage of code splitting with dynamic imports without using ECMAScript modules (more or less, from a high level view).

Of course I'm okay changing whatever wording as needed, but I think we should really point out that require() and import are two very different beasts, and I am happy to write up as to way, give links, etc.

Hello!

I just want to point something out here:

Using the commonjs format (require() and module.exports, though node does add some additional things on top of that spec) *is not* the same, or even roughly the same, which is implied/stated by this line here:

>Instead of using require and module.exports like shown above, JavaScript has an alternate syntax, which is a more accepted standard. Choose whichever you want: they do the same thing.

I propose a minor edit here, which is that we actually want to clarify that the ECMAScript standard that has been finalized (often this is known as ES6 or ES2015. There was a minor revision in there but thats long since passed in either case) and one of the purposes, per the Module spec, is that this is the standard for which modules will be *native to the browser*. (In the evergreen versions of Safari, Chrome, Firefox, and I think even Edge now, you can use `<script type="module" src="somemodule.js">` and it will work fine). They are also the only official way of using dynamic imports, as highlighted [in your own documentation](https://symfony.com/doc/current/frontend/encore/code-splitting.html) in a way that I think gets the point across very well.

I think this is very misleading otherwise. You do in fact see a performance decoupling bundling the commonjs modules, as webpack can't take advantage of code splitting with dynamic imports without using ECMAScript modules (more or less, from a high level view).

Of course I'm okay changing whatever wording as needed, but I think we should really point out that require() and import are two *very* different beasts, and I am happy to write up as to way, give links, etc.
Copy link
Member

@javiereguiluz javiereguiluz 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 this proposal! I agree with it, but I proposed some minor reword to it. What do you think?

@@ -180,8 +180,10 @@ The import and export Statements
--------------------------------

Instead of using ``require`` and ``module.exports`` like shown above, JavaScript
has an alternate syntax, which is a more accepted standard. Choose whichever you
want: they do the same thing.
has an alternate syntax, which is the new finalized ECMAScript standard for using modules in the browser, including
Copy link
Member

Choose a reason for hiding this comment

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

I propose the following reword to not say "the new finalized" (because that expression won't age well):

Instead of using ``require`` and ``module.exports`` like shown above, JavaScript
provides an alternate syntax based on the `ECMAScript 6 modules`_ that includes
the ability to use dynamic imports.

And at the bottom of the article, the URL for the "ECMAScript 6 modules" link must be added:

.. _`ECMAScript 6 modules`: https://hacks.mozilla.org/2015/08/es6-in-depth-modules/

@@ -333,8 +335,7 @@ If you want to only compile a CSS file, that's possible via ``addStyleEntry()``:
Encore
// ...

.addStyleEntry('some_page', './assets/css/some_page.css')
;
.addStyleEntry('some_page', './assets/css/some_page.css');
Copy link
Member

Choose a reason for hiding this comment

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

Let's revert this change because in most examples the Encore config has multiple lines and we always put the last ; in a new line. Thanks!

@javiereguiluz
Copy link
Member

@ProtonScott thanks for this improvement and congrats on your first Symfony Docs contribution! We merged it in 3.4 branch.

javiereguiluz added a commit that referenced this pull request Dec 3, 2018
… javiereguiluz)

This PR was submitted for the 4.1 branch but it was merged into the 3.4 branch instead (closes #10692).

Discussion
----------

Changing the wording around module.exports

Hello!

I just want to point something out here:

Using the commonjs format (require() and module.exports, though node does add some additional things on top of that spec) *is not* the same, or even roughly the same, which is implied/stated by this line here:

>Instead of using require and module.exports like shown above, JavaScript has an alternate syntax, which is a more accepted standard. Choose whichever you want: they do the same thing.

I propose a minor edit here, which is that we actually want to clarify that the ECMAScript standard that has been finalized (often this is known as ES6 or ES2015. There was a minor revision in there but thats long since passed in either case) and one of the purposes, per the Module spec, is that this is the standard for which modules will be *native to the browser*. (In the evergreen versions of Safari, Chrome, Firefox, and I think even Edge now, you can use `<script type="module" src="somemodule.js">` and it will work fine). They are also the only official way of using dynamic imports, as highlighted [in your own documentation](https://symfony.com/doc/current/frontend/encore/code-splitting.html) in a way that I think gets the point across very well.

I think this is very misleading otherwise. You do in fact see a performance decoupling bundling the commonjs modules, as webpack can't take advantage of code splitting with dynamic imports without using ECMAScript modules (more or less, from a high level view).

Of course I'm okay changing whatever wording as needed, but I think we should really point out that require() and import are two *very* different beasts, and I am happy to write up as to way, give links, etc.

<!--

If your pull request fixes a BUG, use the oldest maintained branch that contains
the bug (see https://symfony.com/roadmap for the list of maintained branches).

If your pull request documents a NEW FEATURE, use the same Symfony branch where
the feature was introduced (and `master` for features of unreleased versions).

-->

Commits
-------

d8ba869 Minor tweaks
577e35e Changing the wording around module.exports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants