Babel v7 compatibility #3

jaydenseric · 2018-03-19T01:29:19Z

With this update, the package should be compatible with both Babel v6 and v7.

Updated dependencies.
Removed the unused simple-assign dependency.
Plugin options now an object instead of a string (Babel v7 requirement).
Option is called moduleSpecifier:
- This is how it is called in the modules spec.
- Options default to use object-assign:
  - It's the most popular implementation.
  - IMO the strongest use case for babel-plugin-transform-replace-object-assign is to optimize React projects by configuring all plugins to output the Object.assign builtin (instead of helpers), then instead of polyfilling, replace all occurrences with the same object-assign dependency used by React.
declar to declare variable name typo fix.
Uses @babel/helper-module-imports to fix Support for CommonJS? #1. The output is either ESM or CJS depending if sourceType is module or script.
Updated tests and fixtures. Babel v7 outputs slight differences.
Disabled and ignored package-lock.json. Package lock files are for apps, not packages; npm blacklists them from ever being published.
Updated readme for the new API.
Replaced lodash.assign in readme examples with object-assign. It looks more intuitive and demos the defaults.

- Updated dependencies. - Plugin options now an object instead of a string (Babel v7 requirement). - Options default to use `object-assign`. - Replaced `lodash.assign` in examples with `object-assign`. - Updated tests and fixtures. - Updated readme.

Package lock files are for apps, not packages; npm blacklists them from ever being published.

jaydenseric · 2018-03-19T01:52:55Z

Please don't merge until I have a chance to integrate @babel/helper-module-imports, to fix #1.

Fixes newoga#1. Also improves the fixuture expected outputs.

newoga · 2018-03-19T03:00:03Z

@jaydenseric This looks great. And looks like you wrapped up integrating @babel/helper-module-imports as well. Let me know if you're wrapped up.

IMO the strongest use case for babel-plugin-transform-replace-object-assign is to optimize React projects by configuring all plugins to output the Object.assign builtin (instead of helpers), then instead of polyfilling, replace all occurrences with the same object-assign dependency used by React.

That's interesting. If you're willing, I think this use case would great to add to the README.md as well.

Thanks for your contribution! 👍

newoga · 2018-03-19T03:04:43Z

FYI I'm wondering what the best plan should be for releasing. Since this is a breaking change, we should consider a major bump (even though the previous version technically never was released at v1).

Since babel v7 is still in beta, I'm wondering whether or not we should publish this in npm has prerelease. That being said, it seems like babel 7 is pretty close to being wrapped up and I would be surprised if there were breaking changes in the plugin API at this point, so may be fine releasing a new stable. Let me know what you think.

jaydenseric · 2018-03-19T03:20:14Z

I think this PR is ready, but won't be sure until I can test it locally on my real project using npm link, as per the React optimization use case. For some reason my AVA tests are erroring with ReferenceError: regeneratorRuntime is not defined, I want to be sure it's not related to this PR.

As for documenting the React optimization use case, I would like to streamline a recipe for it for a while first because the .babelrc.js config for it is not intuitive, here is a WIP:

const { engines: { node } } = require('./package.json')

module.exports = {
  comments: false,
  presets: [
    { plugins: ['babel-plugin-transform-replace-object-assign'] },
    [
      '@babel/env',
      {
        targets: { node: node.substring(2) }, // Strip `>=`
        modules: process.env.MODULE ? false : 'commonjs',
        loose: true
      }
    ],
    ['@babel/preset-react', { useBuiltIns: true }]
  ],
  plugins: [
    [
      '@babel/plugin-proposal-object-rest-spread',
      { loose: true, useBuiltIns: true }
    ],
    ['@babel/proposal-class-properties', { loose: true }]
  ]
}

For the release plan, maybe v1.0.0-alpha.1?

newoga · 2018-03-19T04:02:42Z

I think this PR is ready, but won't be sure until I can test it locally on my real project using npm link, as per the React optimization use case. For some reason my AVA tests are erroring with ReferenceError: regeneratorRuntime is not defined, I want to be sure it's not related to this PR.

Okay, sounds good. Let me know what you find. I haven't moved to babel 7 in my projects yet so I don't have anything real to test this on.

That being said, I did pull this and ran the tests, which all looked good. But I do have some questions regarding the new fixtures. I will leave comments on the changes.

For the release plan, maybe v1.0.0-alpha.1?

Makes sense to me, once we iron out the kinks I'm fine going straight to beta unless we think API could change.

newoga

I think the big question I have is with the test fixtures and making sure the expected.js files reflect what we're trying to test. It seems like the expected result is properly importing the configured Object.assign replacement but is not using it. Let me know what you think.

newoga · 2018-03-19T04:03:54Z

test/fixtures/replace-multiple-object-assign-calls/expected.js


 function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

-(0, _simpleAssign2.default)({ a: 1 }, { b: 2 });
-(0, _simpleAssign2.default)({ c: 3 }, { d: 4 });
+_objectAssign({


@jaydenseric Should this be _simpleAssign if this is working correctly?

I'm super confused. How come the tests used to pass with _simpleAssign expected, when the UID was hardcoded to objectAssign all along?

Yeah, me too. It's be a while since I have looked at this (and babel plugin development in general). But it's possibly that in babel v6, babel always generated the UID for an import based on the package here. The generateUidIdentifier('objectAssign') may have been unnecessary/unused. Can't confirm that at the moment.

jaydenseric · 2018-03-19T04:26:05Z

Finding issues when testing it on my project (although I worked out the regenerator runtime issue was something unrelated).

I was seeing import _objectObject from 'object-assign' instead of _objectAssign.

I don't know why the tests here don't exhibit the issue, but nameHint should be a simple string, which is what I thought generateUidIdentifier did. But it apparently makes an Identifier object, so I think we need to read just the name:

addDefault(
  file.path,
  moduleSpecifier,
  {nameHint: file.get(OBJECT_ASSIGN).name} // <-- This 
)

Since doing that though, now I always get import _objectAssign2 from 'object-assign'.

Note that all along, the instances in code are correctly _objectAssign. Only the default import names are wrong.

newoga · 2018-03-19T04:58:14Z

That's interesting. Yeah, the tests also confuse me. The tests seem to indicate the plugin isn't working.

Since doing that though, now I always get import _objectAssign2 from 'object-assign'.

In your project, out of curiosity, are all the Object.assign calls referencing _objectAssign2?

jaydenseric · 2018-03-19T05:00:29Z

In your project, out of curiosity, are all the Object.assign calls referencing _objectAssign2?

No, with {nameHint: file.get(OBJECT_ASSIGN).name} the calls are _objectAssign() while the import is _objectAssign2.

newoga · 2018-03-19T05:05:38Z

Note that all along, the instances in code are correctly _objectAssign. Only the default import names are wrong.

And sorry if I'm misunderstanding, but you're saying that it is correct behavior for the calls to use _objectAssign? To me I figured it should be referencing the imported _objectAssign2 (which should be your object-assign import). What does _objectAssign reference exactly?

newoga · 2018-03-19T05:21:54Z

@jaydenseric I'm sorry I can't be more helpful regarding the babel v7 API. While the subject matter interests me, I haven't explored it deeply so need to re-familiarize myself. The documentation I'm reading here for babel-helper-module-imports seems to imply that the nameHint options only works for named imports, not default imports via the addDefault method.

Is there any other documentation that says otherwise?

That being said, I'm still puzzled why path.scope.generateUidIdentifier('objectAssign') seemed to work with the different import name in previous versions.

It's past 1am in my timezone. I can investigate this more in the morning. Let me know what you find!

Thanks again @jaydenseric. 😄

jaydenseric · 2018-03-19T06:02:15Z

This is a good reference: https://github.com/babel/babel/blob/master/packages/babel-plugin-transform-runtime/src/index.js#L154

Using prettier. Sorry, I could not live without it.

- Only destructure out the config when it is needed. - No need to set an import name hint.

jaydenseric · 2018-03-19T08:09:23Z

@newoga I think it's ready! Tested it out in the project, and it seems to work ok. Please take a look over the assertions, as I have updated them again; they now look more similar to what you originally had.

jaydenseric · 2018-03-19T08:39:43Z

You can see it in action here: https://github.com/jaydenseric/graphql-react/blob/master/.babelrc.js#L6

newoga · 2018-03-19T13:39:30Z

Just pulled the latest, looks great! 👍And surprised to see the apollo graphql-client to go from 11 KB to 4 KB. I use Apollo's graphql libraries as well. 🎉

I have a few nit-picky things I'd like to add. If you have the time to tackle these I'd really appreciate it, otherwise, I can do them later this evening.

Can we add a minimum version of npm v5 to the package.json engines since we are committing a package.lock file? Also debating just switching this to yarn/yarn.lock but I don't feel super strongly about it.
Can we add @babel/core to the list of peerDependencies since the babel community decided to do that for all their internal plugins?
Can we remove the prettier related development dependencies for now? While I love prettier and think it makes sense to implement, I'm thinking we should add it in a new PR along with some commit hooks using husky/lint-staged and possibly some basic linting. Feel free to do one pass through of prettier on the files (though if you could leave semi-colons on for now, it will make the diff easier to read for actual changes related to the babel 7 implementation).

I think we can do a release today as well. I'm half-tempted to release a new version of this plugin that supports babel v6 with the moduleSpecifier API changes. And then release your babel 7 supported version as v2. I think there are a number of projects waiting for a stable release of babel 7 before upgrading and if any of them depend on this, it will allow them to migrate to the new API before babel 7 is released.

newoga · 2018-03-19T13:41:34Z

Also, forgot to mention, that new implementation of the plugin is super simple and clean. 👍 👍

@jaydenseric

This commit is based on the new API proposed by @jaydenseric in his PR #3. His PR primarily adds support for babel 7. The PR also introduces some necessary breaking changes to the plugin configuration. This commit takes his new API and backports it to the babel 6 supported version of this plugin to ease migrations. The API changes are: 1. Configuration is now optional. If no config is provided, `object-assign` is used. 2. String based configuration is no longer supported. It must be an object with a `moduleSpecifier` key. Documentation was updated and a new test fixture was addded to test the new default configuration behavior.

But first pretty everything, with semicons.

newoga · 2018-03-19T22:52:38Z

@jaydenseric I'm going to cut a v1 release now. Sorry for creating all the extra work for you but after that's done, can you rebase this on top and then we can create a beta release with this (we can do a full release when babel 7 is stable).

newoga · 2018-03-19T23:14:10Z

@jaydenseric I'm all wrapped up. Let's rebase this (might be a bit messy, sorry! 🙏) and merge!

Edit: Essentially for the merge, you can keep your version of all the changes, the only really nasty file might be the package.json which has some changed scripts and maybe the /test/index.js file which has some logic for the new test that was added.

# Conflicts: # README.md # src/index.js # test/index.js Also, fixed tests and prettied code.

# Conflicts: # lib/index.js

newoga · 2018-03-19T23:41:38Z

@jaydenseric All wrapped up? I pulled the latest and everything looks good to me. We just need to add a note in the ## Unreleased changes section about babel v7 compatibility and may adding ^7.0.0-beta.42 as a peerDependency (feel free to do that or I can before I bump the version). And then we can release.

I'm ready to merge when you are though 👍

jaydenseric · 2018-03-19T23:46:04Z

It's ready!

newoga · 2018-03-20T00:05:29Z

Thanks again @jaydenseric. This landed as 2.0.0-beta.0 or you can install via babel-plugin-transform-replace-object-assign@beta.

jaydenseric added 2 commits March 19, 2018 12:09

Updates for Babel v7 compatability.

a9ac1a5

- Updated dependencies. - Plugin options now an object instead of a string (Babel v7 requirement). - Options default to use `object-assign`. - Replaced `lodash.assign` in examples with `object-assign`. - Updated tests and fixtures. - Updated readme.

Disable and ignore package-lock.json.

3a2c1b6

Package lock files are for apps, not packages; npm blacklists them from ever being published.

jaydenseric added 2 commits March 19, 2018 13:40

Use @babel/helper-module-imports.

56b9325

Fixes newoga#1. Also improves the fixuture expected outputs.

Readme JSON example syntax typo fix.

534fa68

newoga requested changes Mar 19, 2018

View reviewed changes

Fixed fixtures.

74cba5d

newoga approved these changes Mar 19, 2018

View reviewed changes

jaydenseric added 3 commits March 19, 2018 19:01

Fixed issues.

1f068eb

Using prettier. Sorry, I could not live without it.

New way to register Babel for Mocha.

5b80892

Simplifications.

04260fa

- Only destructure out the config when it is needed. - No need to set an import name hint.

newoga mentioned this pull request Mar 19, 2018

Implement object based config with default object-assign moduleSpecifier #5

Merged

jaydenseric added 2 commits March 20, 2018 09:45

Remove prettier.

36ab877

But first pretty everything, with semicons.

Added @babel/core peer dependency.

06b258a

jaydenseric added 3 commits March 20, 2018 10:16

Merge branch 'master' into babel-v7

ed66b04

# Conflicts: # README.md # src/index.js # test/index.js Also, fixed tests and prettied code.

Merge branch 'master' into babel-v7

b535663

# Conflicts: # lib/index.js

Changelog typo fix.

ee5bff2

Updated changelog.

9bcafd6

newoga merged commit 17204f3 into newoga:master Mar 19, 2018

jaydenseric deleted the babel-v7 branch March 19, 2018 23:50

jaydenseric mentioned this pull request Apr 10, 2018

Match React’s Object.assign dependency to reduce bundle impact ljharb/prop-types-exact#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Babel v7 compatibility #3

Babel v7 compatibility #3

jaydenseric commented Mar 19, 2018 •

edited

Loading

jaydenseric commented Mar 19, 2018

newoga commented Mar 19, 2018

newoga commented Mar 19, 2018

jaydenseric commented Mar 19, 2018

newoga commented Mar 19, 2018

newoga left a comment

newoga Mar 19, 2018

jaydenseric Mar 19, 2018

newoga Mar 19, 2018

jaydenseric commented Mar 19, 2018

newoga commented Mar 19, 2018

jaydenseric commented Mar 19, 2018

newoga commented Mar 19, 2018 •

edited

Loading

newoga commented Mar 19, 2018

jaydenseric commented Mar 19, 2018

jaydenseric commented Mar 19, 2018

jaydenseric commented Mar 19, 2018

newoga commented Mar 19, 2018 •

edited

Loading

newoga commented Mar 19, 2018

newoga commented Mar 19, 2018

newoga commented Mar 19, 2018 •

edited

Loading

newoga commented Mar 19, 2018 •

edited

Loading

jaydenseric commented Mar 19, 2018

newoga commented Mar 20, 2018

Babel v7 compatibility #3

Babel v7 compatibility #3

Conversation

jaydenseric commented Mar 19, 2018 • edited Loading

jaydenseric commented Mar 19, 2018

newoga commented Mar 19, 2018

newoga commented Mar 19, 2018

jaydenseric commented Mar 19, 2018

newoga commented Mar 19, 2018

newoga left a comment

Choose a reason for hiding this comment

newoga Mar 19, 2018

Choose a reason for hiding this comment

jaydenseric Mar 19, 2018

Choose a reason for hiding this comment

newoga Mar 19, 2018

Choose a reason for hiding this comment

jaydenseric commented Mar 19, 2018

newoga commented Mar 19, 2018

jaydenseric commented Mar 19, 2018

newoga commented Mar 19, 2018 • edited Loading

newoga commented Mar 19, 2018

jaydenseric commented Mar 19, 2018

jaydenseric commented Mar 19, 2018

jaydenseric commented Mar 19, 2018

newoga commented Mar 19, 2018 • edited Loading

newoga commented Mar 19, 2018

newoga commented Mar 19, 2018

newoga commented Mar 19, 2018 • edited Loading

newoga commented Mar 19, 2018 • edited Loading

jaydenseric commented Mar 19, 2018

newoga commented Mar 20, 2018

jaydenseric commented Mar 19, 2018 •

edited

Loading

newoga commented Mar 19, 2018 •

edited

Loading

newoga commented Mar 19, 2018 •

edited

Loading

newoga commented Mar 19, 2018 •

edited

Loading

newoga commented Mar 19, 2018 •

edited

Loading