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

allow extra webpack transform exclusions to be passed #9

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@alunny
Collaborator

alunny commented Nov 13, 2015

fixes #8

Signed-off-by: Andrew Lunny alunny@twitter.com

allow extra webpack transform exclusions to be passed
fixes #8

Signed-off-by: Andrew Lunny <alunny@twitter.com>
Show outdated Hide outdated README.md
@@ -47,6 +48,8 @@ new globalizePlugin({
*output* is the name scheme of the built files.
*transformExclusions* an array of functions to test on filepaths, and optionally exclude matching files from the dependency transformations. See [react-globalize-webpack-plugin](https://github.com/rxaviers/react-globalize-webpack-plugin) for an example usage.

This comment has been minimized.

@rxaviers

rxaviers Nov 17, 2015

Owner

Is react-globalize-webpack-plugin itself the example, or is there an example supposed to be there please?

@rxaviers

rxaviers Nov 17, 2015

Owner

Is react-globalize-webpack-plugin itself the example, or is there an example supposed to be there please?

This comment has been minimized.

@rxaviers

rxaviers Nov 17, 2015

Owner

It's important to mention this only takes effect on Production environment.

@rxaviers

rxaviers Nov 17, 2015

Owner

It's important to mention this only takes effect on Production environment.

This comment has been minimized.

@rxaviers

rxaviers Nov 17, 2015

Owner

Thinking loud... It's not only excluded from the dependency transformations, but also from extracting the formatters and consequently from everything else.

@rxaviers

rxaviers Nov 17, 2015

Owner

Thinking loud... It's not only excluded from the dependency transformations, but also from extracting the formatters and consequently from everything else.

This comment has been minimized.

@rxaviers

rxaviers Nov 17, 2015

Owner

I've finally elaborated my comment at #9 (comment)

@rxaviers

rxaviers Nov 17, 2015

Owner

I've finally elaborated my comment at #9 (comment)

Show outdated Hide outdated README.md
@@ -33,7 +33,8 @@ new globalizePlugin({
developmentLocale: "en", // locale to be used for development.
supportedLocales: [ "en", "es", "zh", ... ], // locales that should be built support for.
messages: "messages/[locale].json", // messages (optional)
output: "globalize-compiled-data-[locale].[hash].js" // build output.
output: "globalize-compiled-data-[locale].[hash].js", // build output.
transformExclusions: [ myCoolFunction ]

This comment has been minimized.

@rxaviers

rxaviers Nov 17, 2015

Owner

Let's include a // (optional)

@rxaviers

rxaviers Nov 17, 2015

Owner

Let's include a // (optional)

Show outdated Hide outdated ProductionModePlugin.js
@@ -65,6 +73,7 @@ ProductionModePlugin.prototype.apply = function(compiler) {
var request = this.state.current.request;
if(param.isString() && param.string === "globalize" &&
!util.isGlobalizeModule(request) &&
!isTransformExcluded(request) &&
!(globalizeCompilerHelper.isCompiledDataModule(request))) {

This comment has been minimized.

@rxaviers

rxaviers Nov 17, 2015

Owner

Can we simplify this and do something like the below (where returning true means process it, false skip it)?

-          !util.isGlobalizeModule(request) &&
-          !isTransformExcluded(request) &&
-          !(globalizeCompilerHelper.isCompiledDataModule(request))) {
+         moduleFilter(request))) {
@rxaviers

rxaviers Nov 17, 2015

Owner

Can we simplify this and do something like the below (where returning true means process it, false skip it)?

-          !util.isGlobalizeModule(request) &&
-          !isTransformExcluded(request) &&
-          !(globalizeCompilerHelper.isCompiledDataModule(request))) {
+         moduleFilter(request))) {

This comment has been minimized.

@rxaviers

rxaviers Nov 17, 2015

Owner

For clarity, I am wondering if we can leverage your change to clean up this former mess.

@rxaviers

rxaviers Nov 17, 2015

Owner

For clarity, I am wondering if we can leverage your change to clean up this former mess.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 17, 2015

Owner

What do you think of the following proposed changes?

  1. Use transformExclusions in development mode too, because there's no prejudice doing so and its usage become more consistent in both modes.
  2. Rename the attribute... (a) I'd like to include filter in its name, because the function is supposed to return a boolean similar to what Array.prototype.filter does and associating the name (with a known API) helps deducing that; (b) I'd like to make the name more generic, it seems like it doesn't only exclude it from transforming the require, but excludes from everything else (the module gets pretty much skipped from globalize plugin processings); A name suggestion: moduleFilter (returning true means process it, returning false means skip it). Open to other suggestions.
  3. Accept a function rather than an Array of functions. The various logics could be handled inside the function. This is a consequence of my proposed change above from transformExclusions (plural) to moduleFilter (singular).
Owner

rxaviers commented Nov 17, 2015

What do you think of the following proposed changes?

  1. Use transformExclusions in development mode too, because there's no prejudice doing so and its usage become more consistent in both modes.
  2. Rename the attribute... (a) I'd like to include filter in its name, because the function is supposed to return a boolean similar to what Array.prototype.filter does and associating the name (with a known API) helps deducing that; (b) I'd like to make the name more generic, it seems like it doesn't only exclude it from transforming the require, but excludes from everything else (the module gets pretty much skipped from globalize plugin processings); A name suggestion: moduleFilter (returning true means process it, returning false means skip it). Open to other suggestions.
  3. Accept a function rather than an Array of functions. The various logics could be handled inside the function. This is a consequence of my proposed change above from transformExclusions (plural) to moduleFilter (singular).
@alunny

This comment has been minimized.

Show comment
Hide comment
@alunny

alunny Nov 17, 2015

Collaborator

I like all of those suggestions. I'll update the change accordingly.

Collaborator

alunny commented Nov 17, 2015

I like all of those suggestions. I'll update the change accordingly.

rename transformExclusions to moduleFilter
plus other review feedback from Rafael

Signed-off-by: Andrew Lunny <alunny@twitter.com>
@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 23, 2015

Owner

I see you have pushed new changes. Note there's no notifications for commit pushes. Please, is it ready for review? Thanks

Owner

rxaviers commented Nov 23, 2015

I see you have pushed new changes. Note there's no notifications for commit pushes. Please, is it ready for review? Thanks

@alunny

This comment has been minimized.

Show comment
Hide comment
@alunny

alunny Dec 15, 2015

Collaborator

Sorry, haven't had a chance to look at this in the last couple of weeks. Will follow up soon :)

Collaborator

alunny commented Dec 15, 2015

Sorry, haven't had a chance to look at this in the last couple of weeks. Will follow up soon :)

@alunny

This comment has been minimized.

Show comment
Hide comment
@alunny

alunny Jan 4, 2016

Collaborator

Hi @rxaviers - sorry for the long delay, but yes this is ready for review.

Collaborator

alunny commented Jan 4, 2016

Hi @rxaviers - sorry for the long delay, but yes this is ready for review.

@@ -33,6 +35,18 @@ module.exports = {
(j !== -1 /* 3 */ && filepath.length - j === 1 /* 4 */);
},
moduleFilterFn: function(filterFn) {

This comment has been minimized.

@rxaviers

rxaviers Jan 13, 2016

Owner

s/filterFn/moduleFilter?

@rxaviers

rxaviers Jan 13, 2016

Owner

s/filterFn/moduleFilter?

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Jan 13, 2016

Owner

Other than the one comment above, it seems good to me.

Owner

rxaviers commented Jan 13, 2016

Other than the one comment above, it seems good to me.

@@ -63,8 +65,7 @@ ProductionModePlugin.prototype.apply = function(compiler) {
// Globalize object.
compiler.parser.plugin("call require:commonjs:item", function(expr, param) {
var request = this.state.current.request;
if(param.isString() && param.string === "globalize" &&
!util.isGlobalizeModule(request) &&
if(param.isString() && param.string === "globalize" && moduleFilter(filepath) &&

This comment has been minimized.

@rxaviers

rxaviers Jan 13, 2016

Owner

s/moduleFilter(filepath)/moduleFilter(request)?

@rxaviers

rxaviers Jan 13, 2016

Owner

s/moduleFilter(filepath)/moduleFilter(request)?

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Jan 13, 2016

Owner

We definitely need some unit tests for this project. 🙈

Owner

rxaviers commented Jan 13, 2016

We definitely need some unit tests for this project. 🙈

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Jan 13, 2016

Owner

I'm amending these two small fixes myself...

Owner

rxaviers commented Jan 13, 2016

I'm amending these two small fixes myself...

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Jan 13, 2016

Owner

Closed by 9211a2a

Owner

rxaviers commented Jan 13, 2016

Closed by 9211a2a

@rxaviers rxaviers closed this Jan 13, 2016

@alunny

This comment has been minimized.

Show comment
Hide comment
@alunny

alunny Jan 13, 2016

Collaborator

Thanks so much @rxaviers :)

Collaborator

alunny commented Jan 13, 2016

Thanks so much @rxaviers :)

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Jan 13, 2016

Owner

Thank you.

Owner

rxaviers commented Jan 13, 2016

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment