Accommodate Babel 6 plugin API #71
Comments
|
I am assuming this is related to this error? 👍 |
|
Correct. I'm creating a pull request as we speak. |
|
After attempting to upgrade this plug-in to work w/ Babel 6, and failing due to other seemingly undocumented breaking changes in Babel's plug-in API, I have reversed course. Babel 6 is a bit of a mess at the moment, and it seems prudent to hold off upgrading to Babel 6 until the dust has settled and the library has stabilized a bit. |
|
Can you document these things you came across for anyone else attempting to make the necessary changes to Babel-plugin-rewire? I was going to take a look myself. |
|
I took a first stab at this by updating the things I know changed. You can see the diff here: Currently getting: |
|
Relevant slack conversation log: |
|
Thanks for Reportage this Bug and the Delay on this issue but I am currently on holidays and will return on the 23rd of november. I will then fix this bug and create a new beta. |
|
In the mean time, I created a new babel plugin that doesn't have the full capability of this plugin, but at least solves our need: https://github.com/TheSavior/babel-plugin-rewire-es5 It will only work for modules that use |
|
+1 |
|
Having just tried to upgrade to Babel6, it seems this is the final blocker in getting it over the line. Rather than just +1'ing, I'm curious how I can help @speedskater ? |
|
This was lost in the sea of annoying +1's, but speedskater said he would fix this after returning from vacation on the 23rd. And indeed there is a branch where work has started (and possibly almost finished). Thanks for all of your hard work @speedskater. |
|
@rnicholus thanks. As you have guessed I am right now working on babel 6. |
|
There is a new beta version available now. 1.0.0-beta-2. Could you all please test it and if you have any issues with it could you please provide a PR with a minimal sample reproducing your error (analog to the samples in the repo used by https://github.com/speedskater/babel-plugin-rewire/blob/master/usage-tests/BabelRewirePluginUsageTest.js. |
|
@speedskater I just wanted to chime in and say thanks for getting a babel-6 compatible version of this library out so quickly. I've tried it in a rather small test-case and it appears to working well so far. Thanks! |
|
@speedskater thanks so much ! I'll test with my app and get back to you. |
|
I've upgraded and all is working well. Thank you. |
|
I can get my tests to run without isparta, but not with.. Anyone else experiencing that issue? (karma+webpack+isparta+rewire) |
|
It is possible to have this beta released on npm.org? (I am looking very much forward to babel 6 support, but is limited to pulling npms from npm.org) |
|
@torarnek the beta is already released? I am myself using And, on another note, the beta3 version works flawlessly for me. Great job! I like the new API direction, too 👍 |
|
@speedskater a little delayed with other things, but in the end |
|
@JoakimLofgren can you provide a concrete minimalistic example. |
|
@speedskater how long do you want to beta test for? Any idea when we might have a final |
|
@doylem I will tackle the bugs occuring in the beta next monday and will than release a new beta. If this works out I will the final 1.0 with beginning of next year. |
|
The beta doesn't seem to play well with babel-polyfill: https://babeljs.io/docs/usage/polyfill/ I get the following error: When I comment out the babel-polyfill, I get no uncaught reference, but then my babel isn't transpiling some of my ES6 features, and my tests fail. Here are my settings: //karma.config.js //tst/entry.js Using "babel-plugin-rewire-es5": "^1.0.2", works btw. |
|
@elayman Thanks for reporting the issue. Could you please create a small sample? I will try to tackle your problem on monday before creating a new beta. |
|
this plugin also doesn't play nice with https://www.npmjs.com/package/babel-plugin-add-module-exports. Babel 6 stopped supporting https://phabricator.babeljs.io/T2212 //something.js
export default {
something: 'whateves'
}
//importer.js
import {something} from './something'; //undefinedinstead import allTheThings from './something';
const {something} = allTheThings;or just //something.js
export const something = 'whatevs';
//we can leave this for "old times sake" if we want
export default {
something
}
//importer.js
import {something} from './something'; //'whatevs'if I try the config below or swap "plugins": [
"rewire",
"add-module-exports",
"transform-runtime",
"transform-decorators-legacy",
"typecheck",
["react-transform",
{
"transforms": [{
"transform": "react-transform-hmr",
"imports": ["react"],
"locals": ["module"]
}, {
"transform": "react-transform-catch-errors",
"imports": ["react", "redbox-react"]
}]
}]
]So for the below example //child-mock.js
export default {
stuff() {
return 'stuff';
},
things() {
return 'things';
}
};
//parent-mock.js
import child from './child-mock';
export default {
callStuff() {
return child.stuff();
},
callThings() {
return child.things();
}
};
//some-spec.js
import mock from '../mocks/parent-mock';
before(() => {
mock.__Rewire__('child', {
stuff() {
return 'rewired stuff';
},
things() {
return 'rewired things';
}
});
});I get the error I already reported here #80 but thought this may get more traffic on the Babel 6 issue thread. "plugins": [
["rewire",
{
"transforms": [{
"transform": "add-module-exports"
}]
}]I'm too up on creating Babel plugins but this seems to be how they accept options I'm guessing. //package.json
"babel-core": "^6.3.17",
"babel-eslint": "^5.0.0-beta6",
"babel-loader": "^6.2.0",
"babel-plugin-add-module-exports": "^0.1.1",
"babel-plugin-react-transform": "^2.0.0-beta1",
"babel-plugin-rewire": "^1.0.0-beta-3",
"babel-plugin-transform-decorators-legacy": "^1.2.0",
"babel-plugin-transform-runtime": "^6.3.13",
"babel-plugin-typecheck": "^3.5.1",
"babel-polyfill": "^6.3.14",
"babel-preset-es2015": "^6.1.18",
"babel-preset-react": "^6.1.18",
"babel-preset-stage-0": "^6.1.18",
"babel-register": "^6.3.13",
"babel-runtime": "^5.8.34", |
|
@speedskater I have updated my comment with code snippets. Thanks. |
|
Hi there! I just tried this out on a couple of projects which use babel-polyfill, babel-register in development mode, and isparta for coverage. Isparta gives me a |
|
@ajbradley import {expect} from 'chai';@speedskater |
|
@srph that would be great. No there is no specific issue about it. Its only because of my (a bit tight ;) ) schedule. |
|
@ajbradley as a quick workaround add |
|
@speedskater @GulinSS Thanks for the response - my concern about this approach (the solution of changing ' if ((typeOfOriginalExport === 'object' || typeOfOriginalExport === 'function') && Object.isExtensible(getLog)) {
addNonEnumerableProperty('__get__', _get__);
addNonEnumerableProperty('__GetDependency__', _get__);
addNonEnumerableProperty('__Rewire__', _set__);
addNonEnumerableProperty('__set__', _set__);
addNonEnumerableProperty('__reset__', _reset__);
addNonEnumerableProperty('__ResetDependency__', _reset__);
addNonEnumerableProperty('__with__', _with__);
addNonEnumerableProperty('__RewireAPI__', _RewireAPI__);
}
... // Removed a lot of template code here for clarity
var _RewireAPI__ = {};Here, while the declaration is hoisted to the top of the scope, AFAIK the initialisation remains at the end. This means that the line: addNonEnumerableProperty('__RewireAPI__', _RewireAPI__);is the same as: addNonEnumerableProperty('__RewireAPI__', undefined);I had changed the templates, adding at the beginning of each: var _RewireAPI__; if ( _RewireAPI__ === undefined) _RewireAPI__ = {};which ensures that only a single initialised value is assigned to the var. I had added this to the start of both the |
|
@ajbradley I think you are right. My currently suggested approach is to change the API to a single exported api object, without nonenumerable exports and modifying the default export. (please see https://gitter.im/speedskater/babel-plugin-rewire). To remove the burden to switch to the new API, I am currently preparing a codemod, which should transfer existing code to the new API. In this case switching to the var version should not cause further issues. Furthermore, statements like addNonEnumerableProperty will not be needed in the future. Regarding your concern of double assignment, the variablename RewiredData_ is generated in a unqiue way (hence, it will not be assigned twice). function __getRewireAPI() {
if(_RewiredData__ === undefined) {
_RewiredData__ = {}
}
return _RewiredData__;What are your opinions on this approach? |
|
Hi all. |
|
I'm not sure I entirely understand the problem (more specifically the part about reexporting default exports). In any case, I think I would be fine with specifying absolute paths, and I think it makes the code more obvious to look at and less magical (as in, you're never wondering if it's guessing stuff and if there could be some name collision somewhere). I recently had to write a test where I had to use the RewireAPI object, and I wasn't yet aware of these particular situations because I had only been mostly rewiring dependencies which had default exports. I find that the code that one ends up with when using this API is not as obvious to understand. I'd be totally fine with doing something like But that's just my personal opinion, as I said, I might be misunderstanding the situation you're referring to :) |
|
@trodrigues Thanks for your feedback. I think I might need to explain the problem a bit more detailed. In an ideal world it would be perfect to write something like this: rewire('path/to/module/').set('exportTarget', stub);The Problem is that somehow we need to access the rewireapi for the path 'path/to/module'. Until now we patched the api on the default export of the module specified in 'path/to/module' and furthermore added a named export. So far so good, this worked in most cases but trouble startes in the following three use cases.
//Both exports in this case provide the rewire api and hence will result in a name clash.
export * from 'path/to/module/b'
export * from 'path/to/module/c'
import * from 'path/to/myredux/reducers';In this case the imported reducers might be polluted with the additional rewire api named import or an additional default export. Unless we find a solution to export the "ModuleAPI" without polluting the current module interface we are stuck, because we cannot not know in which way a code which is instructed by the plugin will be used. What makes matters worse is that we cannot change the generated api to use some kind of ioc paradigm. Namely, let each module register its api. @trodrigues Does this make the problem clearer? |
|
@speedskater Thank you for laying out the problem so clearly. From my experience working on the other rewire packages, there were two points to the philosophy that I want to mention.
So to your point about being unable to rewire a primitive, constant, or frozen javascript object: I don't think this is a big problem. If the user knows they want to rewire it, they can easily write their module in a way that enables rewire to work. In all cases with the other rewire packages, the end user has understood and worked with this limitation without complaint. Reexporting of named/default exports: I think if your module only does: That should work because we can detect that we are already exporting the rewire api, and that it is already attached to the default export. The real interesting hard case is with the double I think this case falls into the above philosophies. If a user wants to rewire something, they need to know this is a limitation and should write things differently to support it. The real challenge here is getting it to not break when it is deeper in node_modules or some external package. Rewire should never break when just running on code. The importing of all reducers is interesting as well. It seems similar to the problem of exporting enumerable properties. @speedskater Do you feel like these problems could be solvable, while less than ideal, if we stated that a limitation of the module was wildcard imports / exports and modules with wildcard imports / exports were not rewire-able? |
|
@TheSavior Thank you for your extensive feedback. You are definitely right that all boils down to the case (which is export * from 'path/to/module/b' as "import *" cases could be worked around. I like your point about users only rewire modules of their own and maybe we should look at the problem from different angle. What if we specify /* @moduleId=/mySpecial/ModuleId/String */ In cases where we have information about the source location on the files system or information about module ids from the modulesystem itself, we could even automatically infer this information. Another problem we should consider is module systems which are different from commonjs like systemjs (which is actually causing a bug as well #93 (comment)), or maybe webpacks 2 new es6 modules (http://www.2ality.com/2015/12/webpack-tree-shaking.html) or upcomming browser implementations. So I think we shouldn't rely on any behaviour of an existing module system (or even run our samples with different module systems). |
|
In general, ES2015 duplicate wildcard exports with conflicts shouldn't throw, right? I believe the last one wins, but doesn't throw due to a conflict. So what that would mean is that the rewire export might not be what you expect, but it should at least not throw, right? My understanding is that the only goal of your approach with the comments it to whitelist the modules to add the rewire API to instead of rewiring every module. If we can do a better job of not adding the rewire api, and just not failing on modules, I'd prefer to keep it low cost to use by not requiring a comment at the top of each file. In regards to the other module systems, I think we should definitely have tests for whatever we support. I'm not familiar with SystemJS in particular, but I don't think webpack tree shaking is a problem for us since I think it would only be a possible problem when using like karma-webpack or something but in test, the export would be used and thus not tree shaken out. |
|
@TheSavior Regarding es2015 duplicate exports as far as i have tested this is not the case. The last one does not win, but it results in an error. the idea of the comment is to generate code like the following RewireSupport.registerModuleAPI('myModuleIdentifierExtractedFromTheComment', RewireAPI);in the actual test you can than lookup the module api by its id without the need to provide an additional export or changing the default export. |
|
@speedskater I don't have the context that you have, so perhaps it is worthwhile to take a step back first. The topic of this issue is purely to work with babel 6. My understanding is that it is not a goal of the ticket to request additional features or capabilities, but to simply work with babel 6. Was there anything inherently different about babel 6 that is keeping us from having the same capabilities as before and just releasing a version that would at least run with babel 6 and not fail in new ways from that which worked with babel 5? If this pain we've started having is purely due to trying to support new es2015 features, perhaps we should separate these two discussions and first get out a new release for people to use as they were, and then figure out these things separately. It would likely require two major version bumps instead of doing it all at once, but this ticket was opened 3 months ago, so perhaps it is worth it. That might also help us understand better what actual changes occurred outside of supporting babel 6's api that is making this break and considering a totally different API layer. |
|
@TheSavior I think you are right, that it would make sense to have a new intermediate babel 6 version out there to remove the pressure of finding a new approach. What do you think? |
|
That all sounds great to me. Re version numbers, I don't particularly care. Version numbers don't really mean anything as long as we follow semver. I don't see any particular reason to stay below 1.0.0 and just bump to 2.0.0 if we have to change the API and can't resolve things without breaking compatibility. |
|
@TheSavior sounds fine for me too. But maybe we should start to deprecate api like Rewire and GetDependency instead of the more familiar get, set |
|
👍 |
|
For prosperity's sake, I was getting the same error @dtothefp was getting (eg File export default () => { }File const mylib = require('./mylib')
//...Threw an error. The resolution was simple, either:
Since I had migrated from Babel 5 to Babel 6 a lot of my imports used Not sure that's related to the core issue of this thread but this was the only search result relevant to the above issue. Hope it helps someone. |
|
@danawoodman thanks for providing the hin. |
|
@ajbradley, @srph To be able to finally publish a working babel 6 version. I reconsidered the tdz issues and replaced RewiredData by a var declaration and moved the declaration of the RewireAPI at the beginning of the generated code. I hope that this fixes all tdz issues. |
|
It took longer as expected but we finally pushed 1.0.0.-rc-1 as a default version to the npm repository. |
|
@speedskater is there a PR or commit you could reference to that addressed this? 😄 |
|
I am using rc1 and getting a plugin error via gulp-babel: TypeError in plugin 'gulp-babel' works fine if i just remove the rewire from .babelrc |
|
I keep getting this error: and the file is: |
and others
Re: http://babeljs.io/blog/2015/10/29/6.0.0/#plugin-api
The text was updated successfully, but these errors were encountered: