Type imports #208

brewsoftware · 2017-09-12T05:15:26Z

The following PR is designed to allow for splitting bolt rule definitions into multiple files. This should enable a few extra features in the bolt ecosystem including:

Allow for re-usable types such as simple validation through publicly available bolt modules.
Code which is easier to read.

This has been requested by the community for some time (#63) and this is a second re-write of the original gulp-firebase-bolt plugin with a more ES6 spec. It is a complete end-to-end integration covering the parse & compiler.

I have explicitly left functions and paths out of this release as I'm running out of time to implement all of that. Not withstanding this there is value in the PR as is.

Example usages include:

import * from './module' // import everything from sub module.
import * as types from './module' // import everything under the 'types' namespace
import {Foo} from './module' // only import specific types
import * from 'module' // global imports

type Bar is Foo {

}

Example usages include:

import * from './module' // import everything from sub module.
import * as types from './module' // import everything under the 'types' namespace
import {Foo} from './module' // only import specific types
import * from 'module' // global imports

type Bar is Foo {

}

Documentation has been updated to include the new spec, tests should be parsing and a sample has been added. An additional fix to the master branch has been cherry picked from #209

Comments are welcome:

Cheers,
Jason
Brew Software.

brewsoftware · 2017-09-12T05:16:34Z

samples/type-imports.bolt

+import { SampleOne } from './subModule/typeOne'
+
+// Lvl 5 - import with an adjusted path
+// import from './subModule/typeOne' at '/somewhere' // duplicate import test


TODO: This will be done in a seperate module.

ThadeuLuz · 2017-09-16T14:34:25Z

Please guys merge this. My current workflow concatenates files :/

wcandillon · 2017-09-27T13:42:16Z

I could really use this feature as well

rockwotj

Hello, thanks for the PR, this looks great!

I'm new to this project, but am hoping to pick up maintenance when I can. I left a few comments, but need to get familiar with the project and get it in a buildable state before we get to merging this. Thank you for your patience.

rockwotj · 2017-10-20T23:05:37Z

bin/firebase-bolt

+* Depricated: Use writeTranslation still needs to be maintained as we have the
+* ability to write the translation from STDIN not just the file system.
+* ??? Global modules should still be supported from std in.
+*/


I would think that if you read from STDIN then you would just resolve relative to the current directory instead of the file location. Thoughts? I'm not sure what the best thing for global imports would be... Maybe they should look in global node packages only?

Yeah, I didn't really think of std in as a use case. Not sure if this is required though. See comments below about global modules.

At least we should gracefully handle errors if people try and do this, but having these two code paths isn't great either.

I think throwing a deprecation warning is appropriate but I wouldn't remove it unless there is a major version bump (backward compatability must be maintained). So I would add this as an open issue to remove old behaviour in the next major version bump.

So 2 suggestions, I will add the deprecation warning and can add the deprecation issue for the next major milestone. (you might have to create the next major milestone tag).

rockwotj · 2017-10-20T23:05:59Z

docs/guide.md

+
+    import * from {'module'}
+
+Each modules is assumed to have an entry point under the 'node_modules/firebase-bolt-module/index.bolt' filename and will be processed from here.


What if they are installed under global node modules?

On global modules I'm not a fan of using them so preference would be to just not support it. It's a bad practice as you lose track of versioning which is what the package manager is designed to do. Some things like the cli might make sense to be global but actual project code should be self contained so that the build chain is stable.

Even with firebase if we have someone who has a v2 & v3 app, which version of the cli am i running to publish and work with the app? if it's installed in the global module space I would have to upgrade everything or nothing which isn't practical.

I agree it's not great to support, we should at least call out that you can't install them globally.

rockwotj · 2017-10-20T23:06:55Z

src/imports-parser.ts

+  var nextFn = nextImport.split('/');
+  let result = '';
+  if (nextFn[0] !== '.' && nextFn[0] !== '..') { // global reference
+    result = './node_modules/' + nextImport + '/index.bolt';


Can we make the assumption that this is always ran from the root of the project directory?

Hmm, interesting point. The difference here is between bower & npm (or npm dedupe).

My preference would be not to have nesting (bower) but I didn't think of the nesting module issue. I can go back to the drawing board and look at the module resolution process to see if we can support an npm hierarchy module resultion.

I don't think we need to support nesting, but maybe I'm not familiar enough is ./node_modules going to exist? Is it possible to run this from a ./src/ directory so that we would need to look in ../node_modules?

node_modules are a standard npm spec so if they use NPM this would exist in the project root directory (not ./src/node_modules).

It's a valid test to load it from the ./src/myfile.bolt directory but generally the root directory is still considered ./

I will have a look at using the following package to do the resolution https://www.npmjs.com/package/node-modules-resolve

or this one is probably better https://www.npmjs.com/package/resolve

Nice. It looks like that first package just recursively searches parent directories until it gets to a ./node_modules and uses that one.

If the root directory is considered to be the current working directory we should check for it when we start. This makes an assumption which isn't enforced anywhere as far as I can tell.

brewsoftware added 3 commits September 10, 2017 08:53

imports update

6297f6d

Linting updates

05378b3

Testing updates

aff5cb3

brewsoftware commented Sep 12, 2017

View reviewed changes

brewsoftware mentioned this pull request Sep 12, 2017

Feature request: Add imports to other Bolt files #63

Open

brewsoftware added 4 commits September 13, 2017 08:25

Simple build fixes for firebase build chain.

8366c0c

Documentation updates.

6ea93da

Added global module and tested in sample import.

9eff946

Fixed logging to pass tests.

d4d4452

rockwotj reviewed Oct 20, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type imports #208

Type imports #208

brewsoftware commented Sep 12, 2017 •

edited

Loading

brewsoftware Sep 12, 2017

ThadeuLuz commented Sep 16, 2017

wcandillon commented Sep 27, 2017

rockwotj left a comment

rockwotj Oct 20, 2017

brewsoftware Oct 21, 2017

rockwotj Oct 21, 2017

brewsoftware Oct 21, 2017

rockwotj Oct 20, 2017

brewsoftware Oct 21, 2017

rockwotj Oct 21, 2017

rockwotj Oct 20, 2017

brewsoftware Oct 21, 2017

rockwotj Oct 21, 2017

brewsoftware Oct 21, 2017

brewsoftware Oct 21, 2017

brewsoftware Oct 21, 2017

rockwotj Oct 21, 2017


		import * from {'module'}

		Each modules is assumed to have an entry point under the 'node_modules/firebase-bolt-module/index.bolt' filename and will be processed from here.

Type imports #208

Are you sure you want to change the base?

Type imports #208

Conversation

brewsoftware commented Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

ThadeuLuz commented Sep 16, 2017

wcandillon commented Sep 27, 2017

rockwotj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brewsoftware commented Sep 12, 2017 •

edited

Loading