Add processModuleFilter and remove postProcessModules #215

cliberal · 2018-08-05T06:55:22Z

Summary
remove postProcessModules hook since it is not working now. Add a processModuleFilter hook to skip some modules before code generating. see #210

Test plan
pass

use processModuleFilter to skip some modules before code generating

codecov-io · 2018-08-05T07:48:27Z

Codecov Report

Merging #215 into master will increase coverage by 0.03%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
+ Coverage   85.79%   85.83%   +0.03%     
==========================================
  Files         137      137              
  Lines        4406     4411       +5     
  Branches      685      686       +1     
==========================================
+ Hits         3780     3786       +6     
+ Misses        554      553       -1     
  Partials       72       72

Impacted Files	Coverage Δ
...ro/src/DeltaBundler/Serializers/sourceMapObject.js	`100% <ø> (ø)`	⬆️
...etro/src/DeltaBundler/Serializers/plainJSBundle.js	`100% <ø> (ø)`	⬆️
...ro/src/DeltaBundler/Serializers/sourceMapString.js	`100% <ø> (ø)`	⬆️
packages/metro-config/src/oldConfig.js	`82.75% <0%> (ø)`	⬆️
packages/metro/src/Server.js	`83.44% <0%> (ø)`	⬆️
packages/metro-config/src/convertConfig.js	`61.53% <0%> (ø)`	⬆️
...etro/src/DeltaBundler/Serializers/deltaJSBundle.js	`100% <100%> (ø)`	⬆️
...es/metro/src/DeltaBundler/Serializers/getAssets.js	`100% <100%> (ø)`	⬆️
...o/src/DeltaBundler/Serializers/getRamBundleInfo.js	`90.24% <100%> (+0.24%)`	⬆️
.../metro/src/DeltaBundler/Serializers/getAllFiles.js	`95% <100%> (+0.55%)`	⬆️
... and 1 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 fde0642...84e119d. Read the comment docs.

facebook-github-bot · 2018-08-05T08:52:18Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2018-08-05T10:04:08Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

rafeca

Thanks for the contribution! having a filtering function is much better than the previous method 👍

Please update the rest of the serializers and we're good to go 😃

rafeca · 2018-08-06T23:09:53Z

docs/Configuration.md

@@ -104,4 +104,4 @@ These options are only useful with React Native projects.
 | `getPolyfills`                  | `({platform: ?string}) => $ReadOnlyArray<string>`                      | An optional list of polyfills to include in the bundle. The list defaults to a set of common polyfills for Number, String, Array, Object... |
 | `postProcessBundleSourcemap`    | `PostProcessBundleSourcemap`                                           | An optional function that can modify the code and source map of the bundle before it is written. Applied once for the entire bundle.        |
 | `getModulesRunBeforeMainModule` | `(entryFilePath: string) => Array<string>`                             | An array of modules to be required before the entry point. It should contain the absolute path of each module.                              |
-| `postProcessModules`            | `(modules: Array<Module>, entryFiles: Array<string>) => Array<Module>` | A function that can change the resulting modules output.                                                                                    |
+| `processModuleFilter`            | `(module: Array<Module>) => boolean` | A function that can change the resulting modules output.                                                                                    |


Can you change the description to: A filter function to discard specific modules from the output?

Sure, it's better

rafeca · 2018-08-06T23:42:36Z

packages/metro/src/DeltaBundler/Serializers/plainJSBundle.js

@@ -42,6 +43,7 @@ function plainJSBundle(
    ...getAppendScripts(entryPoint, graph, options),
  ]
    .filter(isJsModule)
+    .filter(options.processModuleFilter)


This needs to be done to each of the serializers to have consistent results no matter the type of output that the user is generating (it's specially important to update the sourcemap serializers to not end up with incorrect sourcemaps).

👌 I'll have a try. Since I haven't read these files, it may take a while.

cliberal · 2018-08-07T07:18:24Z

Thanks for review

I did not change getAllFiles.js since it does't seem to be used.
There some redundant code but I'm not sure whether can I remove it.

// sourceMapObject.js

function fullSourceMapObject(
  pre: $ReadOnlyArray<Module<>>,
  graph: Graph<>,
  options: {|
    +excludeSource: boolean,
    +processModuleFilter: (module: Module<>) => boolean,
  |},
): BabelSourceMap {
  const modules = [...pre, ...graph.dependencies.values()]
    .filter(isJsModule)
    .filter(options.processModuleFilter) // this line can be removed if we don't use it in `_sourceMapForURL` function below
    .map(module => {
      return {
        ...getJsOutput(module).data,
        path: module.path,
        source: options.excludeSource ? '' : module.getSource(),
      };
    });

  return fromRawMappings(modules).toMap(undefined, {
    excludeSource: options.excludeSource,
  });
}

// Server.js
async _sourceMapForURL(reqUrl: string): Promise<MetroSourceMap> {
    const options: DeltaOptions = this._getOptionsFromUrl(reqUrl);

    const {graph, prepend} = await this._getGraphInfo(options, {
      rebuild: false,
    });

    return sourceMapObject(prepend, graph, {
      excludeSource: options.excludeSource,
      processModuleFilter: this._config.serializer.processModuleFilter,
    });
 }

rafeca · 2018-08-07T10:26:26Z

I did not change getAllFiles.js since it does't seem to be used.

getAllFiles() sadly is being used by Server.getOrderedDependencyPaths(), which sadly is being used by the react-native cli (there's a cli command to get all the files in the bundle).

There some redundant code but I'm not sure whether can I remove it.

The sourceMapObject() method is being used by the Server symbolication entrypoint. This is an http entrypoint http://localhost:8081/symbolicate that allows simulators to display the original file/line number of errors/warnings (you may have noticed that RN development error boxes initially display the raw line number and after a small delay it shows the original file/line/column: this is because the app is making a request to the symbolication entrypoint).

cliberal · 2018-08-07T12:20:10Z

@rafeca thanks for explain that. It's very helpful

rafeca

Thanks for addressing the comments so fast! Looks great now! (sorry for all the repetitive work on the serializers: we're planning create some helpers to be able to reuse logic there.

I'm going to import this PR to our systems

facebook-github-bot

rafeca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cliberal added 3 commits August 5, 2018 13:30

feature: add processModuleFilter and remove postProcessModules

8fab743

use processModuleFilter to skip some modules before code generating

update docs

2308619

update test snap

af2579d

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 5, 2018

rafeca suggested changes Aug 6, 2018

View reviewed changes

featrue: update all serializers compatible with new config option

6b5636e

feat: to compatible with new config option

84e119d

rafeca approved these changes Aug 7, 2018

View reviewed changes

facebook-github-bot reviewed Aug 7, 2018

View reviewed changes

facebook-github-bot closed this in 278afc6 Aug 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add processModuleFilter and remove postProcessModules #215

Add processModuleFilter and remove postProcessModules #215

cliberal commented Aug 5, 2018

codecov-io commented Aug 5, 2018 •

edited

Loading

facebook-github-bot commented Aug 5, 2018

facebook-github-bot commented Aug 5, 2018

rafeca left a comment

rafeca Aug 6, 2018

cliberal Aug 7, 2018

rafeca Aug 6, 2018

cliberal Aug 7, 2018

cliberal commented Aug 7, 2018

rafeca commented Aug 7, 2018

cliberal commented Aug 7, 2018

rafeca left a comment

facebook-github-bot left a comment

Add processModuleFilter and remove postProcessModules #215

Add processModuleFilter and remove postProcessModules #215

Conversation

cliberal commented Aug 5, 2018

codecov-io commented Aug 5, 2018 • edited Loading

Codecov Report

facebook-github-bot commented Aug 5, 2018

facebook-github-bot commented Aug 5, 2018

rafeca left a comment

Choose a reason for hiding this comment

rafeca Aug 6, 2018

Choose a reason for hiding this comment

cliberal Aug 7, 2018

Choose a reason for hiding this comment

rafeca Aug 6, 2018

Choose a reason for hiding this comment

cliberal Aug 7, 2018

Choose a reason for hiding this comment

cliberal commented Aug 7, 2018

rafeca commented Aug 7, 2018

cliberal commented Aug 7, 2018

rafeca left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 5, 2018 •

edited

Loading