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

Feature: Passthru metadata #329

Closed
wants to merge 1 commit into from

Conversation

cogell
Copy link

@cogell cogell commented Nov 11, 2016

Please Read the CONTRIBUTING Guidelines
In particular the portion on Commit Message Formatting

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

note: The following description was lifted from #254 (comment) from the original author @Ognian

  • add metadataSubscribers option as an array of context functions to call
  • pass metadata to metadataSubscribers functions
  • add .idea to .gitignore

This is needed when generating messages for translation from modules:

  • there is the babel-react-intl plugin, which returns the messages it finds in the module in the metadata[react-int] property. (babel returns not only source code and source map but also the ast and the metadata property )
  • since babel loader returns only source code and source map, I extended it to return metadata too. A webpack plugin is now able to aggregate all this metadata...

Does this PR introduce a breaking change?

  • Yes
  • No

If this PR contains a breaking change, please describe the following...

  • Impact:
  • Migration path for existing applications:
  • Github Issue(s) this is regarding:

Other information:

@codecov-io
Copy link

codecov-io commented Nov 11, 2016

Current coverage is 82.31% (diff: 90.90%)

Merging #329 into master will increase coverage by 0.61%

@@             master       #329   diff @@
==========================================
  Files             6          6          
  Lines           153        164    +11   
  Methods          22         25     +3   
  Messages          0          0          
  Branches         33         34     +1   
==========================================
+ Hits            125        135    +10   
  Misses           13         13          
- Partials         15         16     +1   

Powered by Codecov. Last update b2d87f4...dc1c824

@danez
Copy link
Member

danez commented Nov 16, 2016

Thanks for the PR. I cannot see any description on what you are changing and why. Could you elaborate a little bit more?

@danez
Copy link
Member

danez commented Nov 16, 2016

Oh I see there is #254

@danez
Copy link
Member

danez commented Nov 16, 2016

There are a lot of unneeded commits in this PR, can you clean that up?

@cogell
Copy link
Author

cogell commented Nov 16, 2016

@danez cleaned up commits and added the description from #254 into the top PR comment

@cogell cogell force-pushed the passthru_metadata_clean branch 5 times, most recently from 93f8fa6 to dc1c824 Compare November 18, 2016 20:47
- add metadataSubscribers option as an array of context functions to call
- pass metadata to metadataSubscribers functions
- add section about cacheDirectory ENOENT error to README.md
@Ognian
Copy link
Contributor

Ognian commented Jan 17, 2017

@danez @cogell what is now the state of this pr? What can I do to help?

@mbrevda
Copy link

mbrevda commented Feb 8, 2017

@Ognian seems like there is a merge conflict. You may want to clean that up.

@Ognian
Copy link
Contributor

Ognian commented Feb 8, 2017

I'll see what I can do, but it won't be before the end of next week

@Ognian
Copy link
Contributor

Ognian commented Feb 27, 2017

moving to #396 since I have no write access to vogel repo...

@Ognian
Copy link
Contributor

Ognian commented Feb 28, 2017

will create a new pr set up on the new version... and link it to this one...

@danez
Copy link
Member

danez commented Feb 28, 2017

I have to say I'm not sure if this should be merged into babel-loader, this is very specific to the react-intl usecase but the focus of babel-loader is simply making webpack use babel to parse js files. It might be better if the babel plugin would write its data to the disk and then another loader could simply pickup this data.

@Ognian
Copy link
Contributor

Ognian commented Feb 28, 2017

@danez sorry but I totally disagree. Maybe at the moment there is only one babel plugin (although I remember at least another one) extracting meta information and passing it thru to webpack, but actually this is a very basic requirement:
If babel returns 3 arguments: code, map and metadata and webpack can be called with those same 3 arguments than babel-loader should also support them.
Your suggested solution produces not only one file but one file for each source file, producing actually a duplicated source tree for the meta information, and this is just filling up the disk with junk, since webpack compresses this meta information a lot (not to mention that writing to disks slows down the whole thing too).

At the moment I'm rewriting the tests since you are using now ava. Please tell me if you are considering to merge this otherwise it is useless to write tests for something you will not merge...

@Ognian
Copy link
Contributor

Ognian commented Feb 28, 2017

ok moved this to #398

@Ognian Ognian mentioned this pull request Mar 1, 2017
11 tasks
@danez danez closed this Mar 4, 2017
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.

None yet

5 participants