-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add support for simple import *statement* to identifier #2
Conversation
I couldn't get import as an expression to work, so I resorted to define it as a statement for now. I'm pretty sure multi-line imports don't work either and there's no alias functionality yet. So this is still *heavily* WIP.
I have very similar code that I haven't committed yet. But based on lydell's comment I think I need to backtrack. I had created a |
@@ -353,6 +353,7 @@ grammar = | |||
|
|||
Import: [ | |||
o 'IMPORT Expression', -> new Import $2 | |||
o 'IMPORT Assignable FROM Expression', -> new Import $4, $2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to replace Assignable
with something new that represents the ES2015 import syntax, and Expression
with something that only matches '
and "
strings (also keep in mind that interpolation cannot be used here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was a destructured assignment, just like CS already supports (syntactically). Is it not?
Tip: Pretend that CoffeeScript's import/export is entirely different from JavaScript. |
…ROM and IMPORT_AS tokens in lexer
I think I'm getting a hang of how it's supposed to work. 🙌 What works now, as you can see in all the tests that are not commented out and should run through: ```coffee import foo import bar from lib import { foo } from lib import { foo as boo, bar } from lib import { foo, bar } from lib ```
I think I'm getting a hang of how it's supposed to work. 🙌 Dunno how much time I can spend with it the rest of this week though, because I will be in a place without internet (at all, not even mobile) over the weekend. What works now, as you can see in all the tests that are not commented out and should run through: import foo
import bar from lib
import { foo } from lib
import { foo as boo, bar } from lib
import { foo, bar } from lib |
Sure!
|
I have no idea why this is in twice now, it seems Github didn't send it three days ago but tried again now after I did so myself… neat, would be nice if it told me, though. Sorry for the double-post. ;) |
No worries, it's been very helpful. I put your explanation here so it doesn't get lost. Let's keep editing that page as we figure more parts out? I merged your branch into mine and then kept going. I didn't get too far, but everything I've committed I think is an improvement :) especially the more detailed tests and the syntax definition in the comments in And we should maybe find a better place to keep a thread going than this now-closed pull request . . . |
Awesome 👍 Yeah, we could keep hijacking the thread over at coffeescript6/discuss maybe? Or the issue/PR in the jashkenas repo. |
Hey @JimPanic, I think this is probably a better place to discuss than hijacking either of those two threads. I made a few more commits on the We’re down to only two failing
If you look in |
Awesome work! 🙌 I tried to find out what's going on, but I'm not entirely sure. It seems the grammar is correct. I changed a small detail so that it matches the spec to the point and it actually parses the tokens properly. It just seems to not generate the appropriate code from it. I don't have the code handy right now as I've been in meetings half of the day and don't have my work laptop here right now. I'll follow up tomorrow with the debug output I got, maybe you can make sense of it more than I do. Btw, I like the pace we eventually got to! 👍 Feels productive :) |
Okay, take a look at 05aa40b. All the This is actually simpler than the previous version. I realized that everything between the import foo, { bar as baz } from 'lib' # or
import { foo as bar }, baz from 'lib' but not: import foo, { bar }, baz from 'lib' # bad!
import * as foo, { bar }, { baz } from 'lib' # bad! Because of this, I can simplify the arguments that We need many more tests, for longer and more complicated (but still valid) variations; and we need tests that cover multiline |
Niiiiice! 👏 |
Okay, take a look at 0bb6dcd. Most of the export syntaxes are now implemented:
What doesn’t work, because of ambiguities in the grammar:
I’m thinking of excluding these two syntaxes. First, they look suspiciously similar to There are some bigger fish that don’t work, though, namely multiline imports or exports. I commented out the multiline tests that are failing; @JimPanic, do you have time to take a look at the multiline-related code and see if you can figure it out? That code is now living here. |
Oooh, you just beat me to it. I wanted to start implementing the export grammar just now haha. This is great progress! I will take a look and see if I can figure it out. |
Yeah, we should probably figure out a way to not overlap. You know, communication 😃 Though I'm so many time zones behind you that we don't overlap our working hours much. It's all yours for 18 hours at least 😃 |
Okay, I’ve made some progress: If an Exporting a block, or at least the one example I’ve tested, now works: export default (foo) ->
console.log foo becomes export default function(foo) {
return console.log(foo);
}; Though I’m confused by how this works. It’s detected by the grammar Still to do: multiline |
I haven't looked into it yet, no. I had too many meetings to focus on anything the last days. :\ Re |
Two things cause the multiline to break it seems:
|
I have absolutely no idea why those whitespace characters are stripped; shouldn't they be part of the AST? Or maybe we should opt to only outputting single-line imports, even though the source might be multi-line? Because multi-line imports get parser and generated correctly besides the whitespace issue as soon as |
A function is an expression. The grammar is AST:s usually strip or irrelevant information, such as whitespace. Having entire import statements on one line in the output is fine. Perhaps we should do it like object literals, though, which are always multiline in the compiled ouptut, regardless of the input. Don't forget to add support for |
@lydell, per the export spec on MDN, I don’t think it’s practical to test for export default class foo extends bar
baz: ->
console.log 'hello, world!' becomes: var extend = function(child, parent) { for (var key in parent) { if (hasProp.call(parent, key)) child[key] = parent[key]; } function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor(); child.__super__ = parent.prototype; return child; },
hasProp = {}.hasOwnProperty;
export default foo = (function(superClass) {
extend(foo, superClass);
function foo() {
return foo.__super__.constructor.apply(this, arguments);
}
foo.prototype.baz = function() {
return console.log('hello, world!');
};
return foo;
})(bar); (b211a51) Technically this is We have a test for |
@JimPanic, thank you for finding the extraneous import { foo, bar as baz } from 'lib' becomes: import {
foo,
bar as baz
} from 'lib'; which is pretty similar to what you’d get from @lydell, I took your suggestions and simplified our tokens: 43b0248, b211a51, 6b27e23. What else remains to be done? |
Also I want to submit this as a PR soon, even if these edge cases aren’t solved just yet. Should I commit the |
Hmm, how would this handle
and
? I imagine that the first case will fail, which I think it should (though we might want to ensure a helpful error message) and the second case will work just fine, which it also should. Worth adding tests? |
The first example becomes: foo = 1;
export var foo = 2; which Babel converts to: "use strict";
Object.defineProperty(exports, "__esModule", {
value: true
});
exports.foo = foo = 1;
var foo = exports.foo = 2; The second example becomes: export var foo = 2;
foo = 3; which Babel converts to: "use strict";
Object.defineProperty(exports, "__esModule", {
value: true
});
var foo = exports.foo = 2;
exports.foo = foo = 3; So both are valid JavaScript, but both end up with |
It’s fine to create a PR even if you’re not 100% finished. That’ll attract more reviewers. Yes, do commit the compiled .js files. Documentation is usually written right before a release. I’d leave that out from your PR. |
Okay, 1e0714e passes all the I also added several more tests regarding variable assignment, like: foo = 5
import { bar } from 'lib'
baz = 7 which should produce just a Also I don’t think |
my hunch is setting a flag at an
hmm, doesn't seem desirable. per @lydell 's comments above, seems fine to submit the PR without that working, but might be good to write failing tests describing better behavior first. I'll try to do that today if I can. |
@rattrayalex, there are failing tests in 1e0714e. The code inside lexer.coffee |
Assuming you meant Let's see, the grammar is |
I see I know I at least have gotten my |
ah, as for detecting |
(made a first stab at resolving this, linked to a patch in 7a3113e#commitcomment-18643729 ) |
Wow, amazing how much all of you have done since I left for vacation just on Friday! Kudos! 🔝 |
cfc18c4 applies @rattrayalex’s patch and makes a few tweaks. All the tests now pass. But I’m not sure all of this is the best way to implement things. @lydell, perhaps you could provide some guidance here. So @rattrayalex got this to work: o 'EXPORT Identifier = Expression', -> new Export new Assign $2, $4, 'export' but further down there’s syntax like this (as if I applied it to our line): o 'EXPORT Identifier = Expression', -> new Export new Assign LOC(1)(new Value $2), $4, 'export',
operatorToken: LOC(2)(new Literal $3) What is going on here? Do we need to do any of this—perhaps for proper source maps? What is Also, is |
@lydell my questions in the previous comment still stand, but I revised things in c3893d7. Now instead of using the exports.Assign = class Assign extends Base
constructor: (@variable, @value, @context, options = {}) ->
{@param, @subpattern, @operatorToken, @moduleStatement} = options Since I think what we’re doing here is more similar to how I also set |
Remember that a home made grammar DSL is used in grammar.coffee. All sorts of crazy hacks are used to get it going. All of that is set up at the top of the file. For example,
Regarding I really care about import/export coming to CoffeeScript, so I gladly help out here. Unfortunately, I haven’t really had the time lately to code on it myself. But remember that I am by no means an expert on the CoffeeScript compiler. I started contributing relatively late (the end of 2014 or something) and I’ve learned most things by hacking around, grepping and really trying to understand stuff. Sometimes there are really helpful comments that help, too. So many times when I read your questions I’m like “oh yeah, I remember that thing, but I’m unsure of the details now – I’ll just go grep for it in the source code before I answer – oh wait I need to this first…” |
Well @lydell I appreciate whatever wisdom you have, it’s much more than I have. 😄 Better to at least ask than find out when people point out bugs. I feel like I shouldn’t be so mystified since there’s so much documentation and so many detailed comments, yet grepping through all the comments I don’t see anything that explains a block like this for the isStatement: YES
jumps: THIS
makeReturn: THIS These are just guesses based on an assumption that I guess it’s about time to submit as a PR, and maybe the greater wisdom of a greater crowd might uncover more mistakes? |
As sorta-discussed on another thread, I'm not so sure the |
Oh and thanks very much @GeoffreyBooth for cleaning up + integrating my patch!! Very gratifying 😄 |
@rattrayalex it doesn’t need to be used with |
One thing I just come to think of: Do we disallow import/export in non-toplevel scopes yet? Otherwise that's something that needs to be done. |
@lydell, 3eb4533 adds a check for non-top-level scope. It’s just checking that I have some ideas for more robust testing for this soon-to-be-PR:
Does anyone else have any more serious CoffeeScript apps that currently use modules in some way (CommonJS, backticks, something else)? Especially if your app has tests. I think trying this code on a real app with real modules is the way to really shake out whatever edge cases and bugs remain. |
@GeoffreyBooth that sounds sufficient to me, and probably something that can happen once you've submitted the PR – but if you want more, I might suggest finding some well-tested demo apps out there and replacing |
Okay, I recompiled piqu-ember using my branch, and here’s the diff between the original output via The exercise did catch one bug—I was inadvertently gobbling up all |
I finished the other test, of rewriting the Meteor Todos CoffeeScript example app to use unbackticked
Of course, read init.sh first so you’re not just blindly trusting me. But it’s safe, it doesn’t modify any files outside of the folder created by cloning the repo. |
Nice! Need any help before submitting a PR? |
I don’t think so, just need to find the time to write a proper description. I’ll do it today. Thanks! |
This pull request adds support for ES2015 modules, by recognizing `import` and `export` statements. The following syntaxes are supported, based on the MDN [import](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import) and [export](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export) pages: ```js import "module-name" import defaultMember from "module-name" import * as name from "module-name" import { } from "module-name" import { member } from "module-name" import { member as alias } from "module-name" import { member1, member2 as alias2, … } from "module-name" import defaultMember, * as name from "module-name" import defaultMember, { … } from "module-name" export default expression export class name export { } export { name } export { name as exportedName } export { name as default } export { name1, name2 as exportedName2, name3 as default, … } export * from "module-name" export { … } from "module-name" ``` As a subsitute for ECMAScript’s `export var name = …` and `export function name {}`, CoffeeScript also supports: ```js export name = … ``` CoffeeScript also supports optional commas within `{ … }`. This PR converts the supported `import` and `export` statements into ES2015 `import` and `export` statements; it **does not resolve the modules**. So any CoffeeScript with `import` or `export` statements will be output as ES2015, and will need to be transpiled by another tool such as Babel before it can be used in a browser. We will need to add a warning to the documentation explaining this. This should be fully backwards-compatible, as `import` and `export` were previously reserved keywords. No flags are used. There are extensive tests included, though because no current JavaScript runtime supports `import` or `export`, the tests compare strings of what the compiled CoffeeScript output is against what the expected ES2015 should be. I also conducted two more elaborate tests: * I forked the [ember-piqu](https://github.com/pauc/piqu-ember) project, which was an Ember CLI app that used ember-cli-coffeescript and [ember-cli-coffees6](https://github.com/alexspeller/ember-cli-coffees6) (which adds “support” for `import`/`export` by wrapping such statements in backticks before passing the result to the CoffeeScript compiler). I removed `ember-cli-coffees6` and replaced the CoffeeScript compiler used in the build chain with this code, and the app built without errors. [Demo here.](https://github.com/GeoffreyBooth/coffeescript-modules-test-piqu) * I also forked the [CoffeeScript version of Meteor’s Todos example app](https://github.com/meteor/todos/tree/coffeescript), and replaced all of its `require` statements with the `import` and `export` statements from the original ES2015 version of the app on its `master` branch. I then updated the `coffeescript` Meteor package in the app to use this new code, and again the app builds without errors. [Demo here.](https://github.com/GeoffreyBooth/coffeescript-modules-test-meteor-todos) The discussion history for this work started [here](#4160) and continued [here](GeoffreyBooth#2). @lydell provided guidance, and @JimPanic and @rattrayalex contributed essential code.
I couldn't get import as an expression to work, so I resorted to define it as a
statement for now.
I'm pretty sure multi-line imports don't work either and there's no alias
functionality yet. So this is still heavily WIP.